Closed
Bug 1306478
Opened 8 years ago
Closed 8 years ago
Baldr: arity-insensitive syntax
Categories
(Core :: JavaScript Engine: JIT, defect, P1)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: sunfish, Assigned: sunfish)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
The WebAssembly s-expression syntax changed such that syntactic arity is no longer required to follow actual operator arities, so surprising things like this are valid now: (i32.const 0 (i32.const 1)) (i32.div_s (i32.const 2)) (i32.sub (i32.const 3)) (i32.add) because values are always just pushed on the stack and checked when popped when needed by an operator. Attached is a patch which implements this. It roughly works, though it currently exposes some test failures due to unrelated issues.
Updated•8 years ago
|
Priority: -- → P1
Comment 1•8 years ago
|
||
Is there an update on this bug? It hasn't received attention in the last 19 days? I'm asking since P1 means this should get into the current release. There are only 14 days left before the merge. Is this still a P1? If not should we demote to P2 (next release) or P3 (any release)? If this is still P1 it is starting to become urgent to see some progress.
Updated•8 years ago
|
Flags: needinfo?(sunfish)
Comment 2•8 years ago
|
||
If that's okay, I'll complete this patch and make sure we pass other tests. Leaving it p1.
Flags: needinfo?(sunfish)
Comment 3•8 years ago
|
||
Rebased and updated to fix issues: - initializers expressions want their parenthesis - a test in typecheck.wast (removed since then) is not compatible with arity insensitive parsing Selecting luke as a reviewer, to avoid the conflict of interest author/reviewer.
Attachment #8796321 -
Attachment is obsolete: true
Attachment #8804240 -
Flags: review?(luke)
![]() |
||
Comment 4•8 years ago
|
||
Comment on attachment 8804240 [details] [diff] [review] arity-insensitive.patch Review of attachment 8804240 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: js/src/jit-test/tests/wasm/spec/typecheck.wast @@ +159,5 @@ > (i32.eqz) (drop) > )) > "type mismatch" > ) > +;; Test incompatible with arity insensitive parsing. If I read trunk spec/typecheck.wast correctly, this test was simply removed. Is your intention to just keep this here temporarily and update typecheck.wast as a whole or can you just delete this now?
Attachment #8804240 -
Flags: review?(luke) → review+
Comment 5•8 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #4) > If I read trunk spec/typecheck.wast correctly, this test was simply removed. > Is your intention to just keep this here temporarily and update > typecheck.wast as a whole or can you just delete this now? That's right, it is temporary. Thanks for the quick review!
Updated•8 years ago
|
Summary: baldr: arity-insensative syntax → Baldr: arity-insensitive syntax
Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9ba1705b5af6 BaldrMonkey: support arity-insensitive syntax; r=luke
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9ba1705b5af6
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•