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

Add ability to parse an optional WastCheckType for assert_return_*_nan directives#24

Closed
abrown wants to merge 1 commit intobytecodealliance:masterfrom
abrown:fix-23
Closed

Add ability to parse an optional WastCheckType for assert_return_*_nan directives#24
abrown wants to merge 1 commit intobytecodealliance:masterfrom
abrown:fix-23

Conversation

@abrown
Copy link
Member

@abrown abrown commented Nov 4, 2019

@alexcrichton, please take a look but we can wait to merge until I hear from @Honry if WAVM moves to this style. Things I was not too confident about:

  • I made ParseBuffer::parser public so I could write a test but there may be a less API-altering way to do this
  • I added a RParen type so I could create a helper for parsing Option<WastCheckType>s; I see the impl for Option in parser.rs but I don't think the logic in there would work (?).

…n directives; closes bytecodealliance#23

Prior to this, WAST code like `(assert_return_canonical_nan (invoke ...) f32x4)` would fail. This change optionally parses the final `f32x4` to enable both pre-existing tests that do not include this and future tests that will include it.
@alexcrichton
Copy link
Member

👍

Lemme know how the wavm discussion goes and we can merge this once that's sorted out!

@abrown
Copy link
Member Author

abrown commented Nov 11, 2019

@Honry, any update from WAVM on whether there will be a change to these directives?

@Honry
Copy link

Honry commented Nov 12, 2019

Still waiting for @AndrewScheidecker's response. See WAVM/WAVM#187 (comment)

@abrown
Copy link
Member Author

abrown commented Nov 12, 2019

@alexcrichton, I created a spec issue for this; hopefully we can get some consensus there and then merge this.

@alexcrichton
Copy link
Member

I'm seeing updates like WebAssembly/testsuite#9 include directives like assert_return_canonical_nan_f64x2, so @abrown wanna go ahead and land that and we can always update the syntax later if it changes?

@abrown
Copy link
Member Author

abrown commented Nov 18, 2019

Well, it sounds like in WebAssembly/simd#137 we might just change the syntax right away. So I started a WIP PR to do just that in WebAssembly/simd#142. So I'm more interested in getting that done first and coming back to this especially since we can control which SIMD spec tests get run on a per-file basis in wasmtime.

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.

3 participants