Closed
Bug 1249524
Opened 8 years ago
Closed 8 years ago
Baldr: add the other (param) declaration form to the text format
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: luke, Assigned: luke)
Details
Attachments
(1 file)
8.87 KB,
patch
|
mbx
:
review+
|
Details | Diff | Splinter Review |
It's in ml-proto and Binaryen uses it.
Attachment #8721146 -
Flags: review?(mbebenita)
Comment 1•8 years ago
|
||
Comment on attachment 8721146 [details] [diff] [review] multi-param Review of attachment 8721146 [details] [diff] [review]: ----------------------------------------------------------------- Small fixes needed, looks good otherwise. ::: js/src/asmjs/WasmText.cpp @@ +431,4 @@ > : name_(name), > sig_(sig), > varTypes_(Move(varTypes)), > + localNames_(Move(localNames)), We should assert that localNames.length() == varTypes.length() + sig.args().length(); @@ +2553,5 @@ > +static bool > +ParseLocal(WasmParseContext& c, WasmNameVector* localNames, WasmAstValTypeVector* localTypes) > +{ > + if (c.ts.peek().kind() == WasmToken::Name) { > + if (!localNames->append(c.ts.getIfName())) We need to append a name even if it doesn't have one, so we can map names to indices correctly. @@ +2578,5 @@ > static WasmAstFunc* > ParseFunc(WasmParseContext& c, WasmAstModule* module) > { > + WasmAstValTypeVector varTypes(c.lifo); > + WasmAstValTypeVector argTypes(c.lifo); If we rename this then we should also rename WasmAStSig::args_ and other WasmAstValTypeVector members that store types. @@ +2604,5 @@ > while (c.ts.getIf(WasmToken::OpenParen)) { > WasmToken token = c.ts.get(); > switch (token.kind()) { > case WasmToken::Local: > + if (!ParseLocal(c, &localNames, &varTypes)) (param ..) and (local ..) s-expressions may actually be interleaved. This only works if we assume that they can't. @@ +3141,2 @@ > for (size_t i = 0; i < numVars; i++) { > + if (!r.registerVarName(func.localNames()[i], i)) I don't think this works. Suppose you have: (local $a i32) (local i32) (local $b i32) (get_local $a) (get_local 1) (get_local $b) The code now maps: $a -> 0, $b -> 1, but it should be $a -> 0, $b -> 2, otherwise (get_local 1) (get_local $b) alias the same local.
Attachment #8721146 -
Flags: review?(mbebenita) → review+
Assignee | ||
Comment 2•8 years ago
|
||
(In reply to Michael Bebenita [:mbx] from comment #1) > We should assert that localNames.length() == varTypes.length() + > sig.args().length(); That's a great idea, but sig is unfortunately a WasmRef, not yet a Sig. > We need to append a name even if it doesn't have one, so we can map names to > indices correctly. Good catch! Added a test case for this. > If we rename this then we should also rename WasmAStSig::args_ and other > WasmAstValTypeVector members that store types. I'll put 'em back; seems clear enough without the suffix. > (param ..) and (local ..) s-expressions may actually be interleaved. This > only works if we assume that they can't. Yeah, that's a good point and a pre-existing issue. I'll add "if (!locals.empty()) fail" in the Param case so this doesn't parse since this is grammatically weird anyhow (text order doesn't match index order).
Comment 4•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/78bf144d2e46
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in
before you can comment on or make changes to this bug.
Description
•