-
Notifications
You must be signed in to change notification settings - Fork 42
[test] Fix issue #181 #183
Conversation
- Fix mistake tests in simd_load.wast - Missing lane index should be malformed - Test more values of lane index
|
@alexcrichton, @abrown, @tlively, PTAL, thanks! |
test/core/simd/simd_lane.wast
Outdated
|
|
||
| ;; Malformed lane index value | ||
|
|
||
| (assert_malformed (module (func (result i32) (i8x16.extract_lane_s -1 (v128.const i8x16 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0)))) "malformed lane index") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be misinterpreting the intention, but I was under the impression that assert_malformed should largely always be used with (module quote "...") because in theory this sort of parse error doesn't work in that when we try to parse the assert_malformed directive it will fail since the contents are designed to not parse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WAVM is more permissive here than it probably should be: an error in an unquoted module that the parser can recover from without skipping out of the assert_malformed will be treated the same as if the module was quoted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Searching around https://github.com/webassembly/testsuite it looks like all assert_malformed directives are either coupled with (module quote or (module binary currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexcrichton @Honry WAVM/WAVM@165863c makes it an error to use anything other than a quoted or binary module in assert_malformed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! That looks great to me, thanks! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @AndrewScheidecker, @alexcrichton, I will make a copy of the fix from WAVM to this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
|
@Honry, looks good to me. @alexcrichton, should we revert bytecodealliance/wat#45 then? |
Fix #181