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

Add support for nan:canonical and nan:arithmetic in assert_return#42

Merged
alexcrichton merged 9 commits intobytecodealliance:masterfrom
abrown:fix-41
Jan 9, 2020
Merged

Add support for nan:canonical and nan:arithmetic in assert_return#42
alexcrichton merged 9 commits intobytecodealliance:masterfrom
abrown:fix-41

Conversation

@abrown
Copy link
Member

@abrown abrown commented Jan 9, 2020

This creates a new AssertExpression type that parses [type].const [value] expressions in a way that allows the nan:canonical and nan:arithmetic patterns to appear in these expressions; this is separate from the Expression implementation to disallow using these patterns in regular code (they are only valid in assert_return directives).

@abrown
Copy link
Member Author

abrown commented Jan 9, 2020

@alexcrichton, this is still a WIP. Here are some questions I had as I started this:

  • I implemented this completely separately from expr.rs--it felt less invasive; thoughts?
  • How can I use my repeat*! macros to avoid repeating parser.parse()?? When I use them Rust thinks the type is [unknown; 1] and barfs.
  • What's the right way to do lifetimes for what I'm trying to do in the tests? I see this type of error:
   --> crates/wast/src/ast/assert_expr.rs:152:15
    |
150 |     fn parse_into<'a, T>(text: &'static str) -> T where T: Parse<'a> {
    |                   -- lifetime `'a` defined here
151 |         let buffer = ParseBuffer::new(text).unwrap();
152 |         parse(&buffer).unwrap()
    |         ------^^^^^^^-
    |         |     |
    |         |     borrowed value does not live long enough
    |         argument requires that `buffer` is borrowed for `'a`
153 |     }
    |     - `buffer` dropped here while still borrowed
  • Other suggestions? Future commits will add the use of AssertExpression to the AssertReturn directive, e.g.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

it felt less invasive; thoughts?

Looks good to me!

What's the right way to do lifetimes for what I'm trying to do in the tests?

I think it's fine to skip the tests here since this repository already parses all the *.wast files. Otherwise I'd add infrastructure to parse a *.wast file itself rather than adding a parsing function.

@abrown abrown marked this pull request as ready for review January 9, 2020 21:51
This creates a new AssertExpression type that parses `[type].const [value]` expressions in a way that allows the `nan:canonical` and `nan:arithmetic` patterns to appear in these expressions; this is separate from the `Expression` implementation to disallow using these patterns in regular code (they are only valid in `assert_return` directives).
abrown and others added 5 commits January 9, 2020 14:19
We have exact v128 type information on the old assertions so we can
avoid using the legacy variant. Additionally ensure that the legacy
variant indicates canonical/arithmetic nans.
@alexcrichton alexcrichton merged commit b0e8d9f into bytecodealliance:master Jan 9, 2020
abrown added a commit to abrown/wasmtime that referenced this pull request Jan 10, 2020
Due to API changes in wast, this removes all AssertReturn* directives and expands the functionality the single AssertReturn directive with the `nan:canonical` and `nan:arithmetic` patterns. See bytecodealliance/wat#42 and WebAssembly/spec#1104 for context.
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