Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement has_feature. #120

Merged
merged 5 commits into from
Oct 16, 2015
Merged

Implement has_feature. #120

merged 5 commits into from
Oct 16, 2015

Conversation

sunfishcode
Copy link
Member

This implements has_feature as described in the design.

@sunfishcode sunfishcode force-pushed the has-feature branch 3 times, most recently from e1a223d to a43d0c4 Compare October 14, 2015 17:41
@sunfishcode
Copy link
Member Author

This is now rebased on top of the host opcode grouping changes, which required teaching the host opcode code about host opcodes that don't need memory.

@kg
Copy link
Contributor

kg commented Oct 14, 2015

It would be cool to add a known-always-present feature so we can test both the positive and negative case in the automated test. That's a pretty small thing though. Otherwise this LGTM.

@sunfishcode
Copy link
Member Author

Good idea. I added a feature called "wasm" which is claimed to be unconditionally supported :-).

@@ -90,10 +90,17 @@ let type_cvt at = function
| DemoteFloat64 -> error at "invalid conversion"
), Float64Type

type type_hostop_type = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of introducing another variant of function type for a singular use, can't type_hostop just return a pair func_type * bool?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works.

@rossberg
Copy link
Member

I'm afraid I still don't understand how a feature test inside the language is supposed to make sense -- AFAICS it is utterly useless as long as there's no defined way to actually handle unsupported features inside the language as well. And I have no idea how that would be done or specced or implemented. The design docs don't explain that either, only how to do feature testing on the meta level (which doesn't require the operator). It seems to me that putting in the operator now is starting with a (useless) step 2 before we have a clue about step 1.

But other than these fundamental doubts about the premise, and the nits above, LGTM. :)

@lukewagner
Copy link
Member

@rossberg-chromium The intended usage I'm anticipating in the MVP is described here.

@rossberg
Copy link
Member

@lukewagner, yes, that's what I looked at, but as far as the MVP is concerned, the only workable suggestions there are doing meta-level feature detection (or am I misreading that?). So what still puzzles me is how in-language feature detection is useful before you also add the actual ability to "decode-through" unrecognised features?

@lukewagner
Copy link
Member

@rossberg-chromium Because we'd want implement layer 1 decoder (which also performed the polyfilling) in wasm (using only MVP features). That (tiny) module needs to learn what features are supported somehow, so might as well be from within wasm; otherwise we'd have to add it as a JS function and that seems weird.

@rossberg
Copy link
Member

I see! Okay, sorry that I didn't get that from the description. I wouldn't mind clarifying this use case (and perhaps moving the explanation to the FeatureTest page itself). ;)

@lukewagner
Copy link
Member

@rossberg-chromium Will do; I forgot we even had FeatureTest.md :)

@kg
Copy link
Contributor

kg commented Oct 15, 2015

So what still puzzles me is how in-language feature detection is useful before you also add the actual ability to "decode-through" unrecognised features?

For the record, my binary encoding proposal already covers being able to decode unrecognized AST nodes.

@sunfishcode
Copy link
Member Author

The patch is updated to address the comments above. Any further comments?

@rossberg
Copy link
Member

lgtm

sunfishcode added a commit that referenced this pull request Oct 16, 2015
@sunfishcode sunfishcode merged commit 9c7dbbc into master Oct 16, 2015
@sunfishcode sunfishcode deleted the has-feature branch October 16, 2015 14:56
alexcrichton pushed a commit to alexcrichton/spec that referenced this pull request Nov 18, 2019
Integrate latest spec tests from WAVM
eqrion pushed a commit to eqrion/wasm-spec that referenced this pull request Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants