-
Notifications
You must be signed in to change notification settings - Fork 27
Text #119
Conversation
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.
LGTM with nits.
I'm mostly wondering about some tests you added.
test/core/elem.wast
Outdated
(elem (table $t) (i32.const 0) funcref) | ||
(elem (table $t) (i32.const 0) funcref (ref.func $f) (ref.null)) | ||
(elem (table $t) (i32.const 0) func) | ||
(elem (table $t) (i32.const 0) func $f $g) |
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.
Aren't these just duplicates of the tests above?
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.
Oops, yes, removed (here and below).
(data $d3 (offset (i32.const 0))) | ||
(data $d4 (offset (i32.const 0)) "" "a" "bc" "") | ||
(data $d5 (memory 0) (i32.const 0)) | ||
(data $d6 (memory 0x0) (i32.const 1) "a" "" "bcd") | ||
(data $d7 (memory 0x000) (offset (i32.const 0))) | ||
(data $d8 (memory 0) (offset (i32.const 0)) "" "a" "bc" "") | ||
(data $d9 (memory $m) (i32.const 0)) | ||
(data $d10 (memory $m) (i32.const 1) "a" "" "bcd") | ||
(data $d11 (memory $m) (offset (i32.const 0))) | ||
(data $d12 (memory $m) (offset (i32.const 0)) "" "a" "bc" "") |
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.
These tests look exactly the same as above, although with IDs. Do you expect these additional data segments to increase coverage? Could you add a comment which describes the goals of these tests?
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.
Yes. Note that the header comment above says "Syntax", so this module is all about testing that the parser understands all combinations of syntax.
test/core/elem.wast
Outdated
(elem $a1 (table $t) (i32.const 0) funcref) | ||
(elem $a2 (table $t) (i32.const 0) funcref (ref.func $f) (ref.null)) | ||
(elem $a3 (table $t) (i32.const 0) func) | ||
(elem $a4 (table $t) (i32.const 0) func $f $g) | ||
(elem $a5 (table $t) (i32.const 0) funcref) | ||
(elem $a6 (table $t) (i32.const 0) funcref (ref.func $f) (ref.null)) | ||
(elem $a7 (table $t) (i32.const 0) func) | ||
(elem $a8 (table $t) (i32.const 0) func $f $g) | ||
(elem $a9 (table $t) (offset (i32.const 0)) funcref) | ||
(elem $a10 (table $t) (offset (i32.const 0)) func $f $g) | ||
(elem $a11 (table 0) (i32.const 0) func) | ||
(elem $a12 (table 0x0) (i32.const 0) func $f $f) | ||
(elem $a13 (table 0x000) (offset (i32.const 0)) func) | ||
(elem $a14 (table 0) (offset (i32.const 0)) func $f $f) | ||
(elem $a15 (table $t) (i32.const 0) func) | ||
(elem $a16 (table $t) (i32.const 0) func $f $f) | ||
(elem $a17 (table $t) (offset (i32.const 0)) func) | ||
(elem $a18 (table $t) (offset (i32.const 0)) func $f $f) | ||
(elem $a19 (offset (i32.const 0))) | ||
(elem $a20 (offset (i32.const 0)) funcref (ref.func $f) (ref.null)) | ||
(elem $a21 (offset (i32.const 0)) func $f $f) | ||
(elem $a22 (offset (i32.const 0)) $f $f) | ||
(elem $a23 (i32.const 0)) | ||
(elem $a24 (i32.const 0) funcref (ref.func $f) (ref.null)) | ||
(elem $a25 (i32.const 0) func $f $f) | ||
(elem $a26 (i32.const 0) $f $f) |
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.
Also here you add tests for active element segments with IDs which are the same as the tests above. What is the purpose of these tests? As far as I understand, you cannot even use active elements by ID. Can you add a comment which explains the idea behind these tests, and how they are supposed to increase coverage?
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.
Like above, this is purely a syntax test, see heading.
Active elements can be used by ids, but behave like a dropped segment, i.e., trap in most cases.
Fix a couple of parser omissions regarding elem & data syntax. Refactor grammar in spec slightly for more clarity. Adjust tests.
Baseline is #115. See last commit for new changes.