Skip to content
This repository was archived by the owner on Jun 16, 2020. It is now read-only.

Add no_std support. #49

Merged
merged 2 commits into from
Mar 28, 2018
Merged

Conversation

sunfishcode
Copy link
Member

This adds no_std support to wasmparser.rs.

@eternaleye I've attempted to follow your advice in bytecodealliance/cranelift#224 and use features additively, so I'm curious if you have any comments. wasmparser.rs uses HashSet internally, so to enable use of hashmap_core in an additive way, I've also created a no_std feature which enables that. You can enable both std and no_std at the same time, and it behaves like a std build. But you have to enable at least one of the two.

@eternaleye
Copy link

Ah, that's not quite what I'd meant. 😛

The "purely additive" property is about the exposed API surface; if HashSet is used only internally, then the no_std feature is unnecessary - simply use hashmap_core if the std feature is disabled.

To explain a bit, there are the following possibilities:

  1. Enabling (or disabling) the feature leaves the API surface unchanged
    • This describes "purely internal" use, such as HashSet in this case
  2. Enabling (resp. disabling) the feature expands (resp. shrinks) the API surface
    • For example, an async feature adding a function returning a Future
  3. Enabling (or disabling) the feature replaces an element of the API surface
    • This would cover a case where a public function took or returned a HashSet, and whether the type came from std or hashmap_core was controlled by a feature
  4. Enabling (resp. disabling) the feature shrinks (resp. expands) the API surface
    • This would cover a no_std feature that, say, did away with methods that return Vec

1 and 2 are kosher; 3 and 4 are not.

@sunfishcode
Copy link
Member Author

@eternaleye The problem that I ran into is that I couldn't find a way to tell Cargo to add hashmap_core as a dependency in non-std builds and not in std builds. Do you know of a way to do that?

@eternaleye
Copy link

@sunfishcode:

[target.'cfg(not(feature = "std"))'.dependencies]
hashmap_core = "0.1.2"

@sunfishcode
Copy link
Member Author

@eternaleye When I try that, it pulls in hashmap_core for std builds too.

$ git diff
diff --git a/Cargo.toml b/Cargo.toml
index 08bc406..aead01a 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -12,17 +12,12 @@ exclude = ["fuzz/"]
 
 [dependencies]
 
-[dependencies.hashmap_core]
-version = "0.1.1"
-optional = true
+[target.'cfg(not(feature = "std"))'.dependencies]
+hashmap_core = { version = "0.1.2" }
 
 [badges]
 travis-ci = { repository = "yurydelendik/wasmparser.rs" }
 
 [features]
-# The "std" feature enables use of libstd. The "no_std" feature enables use
-# of some minimal std-like replacement libraries. At least one of these two
-# features needs to be enabled.
 default = ["std"]
 std = []
-no_std = ["hashmap_core"]
$ cargo build
   Compiling hashmap_core v0.1.2
[...]

We specifically want to avoid pulling in hashmap_core in std builds.

@eternaleye
Copy link

Ugh, looks like I was mistaken, and there's no way to do that currently.

I would suggest renaming the no_std feature to core, though. Semantically, it "adds support for a core-based environment" rather than negating the std option.

@sunfishcode
Copy link
Member Author

@eternaleye Sounds good, I've renamed no_std to core now. Thanks for your help!

@sunfishcode
Copy link
Member Author

Ok, I'm taking the liberty of merging this. Even though no_std mode is admittedly a little bit black magic right now, it doesn't affect users that don't enable it, and it's not that much clutter in the code. If it turns out to cause problems for anyone, I'd be willing to revert it.

@sunfishcode sunfishcode merged commit 48e252c into bytecodealliance:master Mar 28, 2018
@sunfishcode sunfishcode deleted the no_std branch March 28, 2018 17:46
Robbepop pushed a commit to Robbepop/wasmparser that referenced this pull request Jun 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants