Closed
Bug 1494930
Opened 6 years ago
Closed 6 years ago
[BinAST] Correct differences in emitted bytecode
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: efaust, Assigned: efaust)
Details
Attachments
(4 files)
4.47 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
2.02 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
907 bytes,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
4.78 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
Various corrections/improvements to the bytecode we'll generate from the BinAST parser.
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #9012838 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #9012839 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 4•6 years ago
|
||
Attachment #9012840 -
Flags: review?(arai.unmht)
Comment 5•6 years ago
|
||
Comment on attachment 9012837 [details] [diff] [review] [Part 1] Various fixes Review of attachment 9012837 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BinSource.cpp @@ +221,4 @@ > // TODO (efaust): This capture will have to come from encoder side for arrow functions. > } > > + HandlePropertyName arguments = cx_->names().arguments; can you add a comment which refers PerHandlerParser<ParseHandler>::declareFunctionArgumentsObject ? there are some difference between the implementation here and declareFunctionArgumentsObject. it would be nice to add comment explains why, or maybe just add TODO @@ +221,5 @@ > // TODO (efaust): This capture will have to come from encoder side for arrow functions. > } > > + HandlePropertyName arguments = cx_->names().arguments; > + if (hasUsedName(arguments)) { can you add funbox->bindingsAccessedDynamically() to the condition? https://searchfox.org/mozilla-central/rev/6d1ab84b4b39fbfb9505d4399857239bc15202ef/js/src/frontend/Parser.cpp#2958 https://searchfox.org/mozilla-central/rev/6d1ab84b4b39fbfb9505d4399857239bc15202ef/js/src/frontend/Parser.cpp#2609 @@ +229,5 @@ > + BINJS_TRY(funScope.addDeclaredName(parseContext_, p, arguments, DeclarationKind::Var, > + DeclaredNameInfo::npos)); > + } > + funbox->declaredArguments = true; > + funbox->usesArguments = true; in Parser, above 2 are done only when p is falsy. it should be better doing in the same way, or adding the comment that explains why it's different. @@ +232,5 @@ > + funbox->declaredArguments = true; > + funbox->usesArguments = true; > + > + funbox->setArgumentsHasLocalBinding(); > + if (funbox->bindingsAccessedDynamically()) braces to be clear, it's not necessary to fix it in this patch, if it may cause conflict. it's fine to fix all of them later. you can use jandem's script in bug 1488698 to automatically fix them. @@ +483,4 @@ > } > prefix->setKind(body->getKind()); > prefix->setOp(body->getOp()); > + if (body->hasTopLevelFunctionDeclarations()) and here
Attachment #9012837 -
Flags: review?(arai.unmht) → review+
Updated•6 years ago
|
Attachment #9012838 -
Flags: review?(arai.unmht) → review+
Comment 6•6 years ago
|
||
Comment on attachment 9012839 [details] [diff] [review] [Part 3] Don't generate both deffun and defvar. Review of attachment 9012839 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BinSource.cpp @@ +166,4 @@ > atom = name->name(); > } > > + if (parseContext_ && syntax == FunctionSyntaxKind::Statement) { I don't think parseContext_ can be nullptr here.
Attachment #9012839 -
Flags: review?(arai.unmht) → review+
Comment 7•6 years ago
|
||
Comment on attachment 9012840 [details] [diff] [review] [Part 4] Respect strictness passed in directives Review of attachment 9012840 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/SharedContext.h @@ +229,5 @@ > localStrict = strict; > return retVal; > } > + void forceStrictScript() { > + strictScript = true; can you use this method in Parser? https://searchfox.org/mozilla-central/rev/6d1ab84b4b39fbfb9505d4399857239bc15202ef/js/src/frontend/Parser.cpp#4608
Attachment #9012840 -
Flags: review?(arai.unmht) → review+
Pushed by efaustbmo@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/864b718cd797 Part 1: Various BinAST parser correctness fixes. (r=arai) https://hg.mozilla.org/integration/mozilla-inbound/rev/5fb50ca3776e Part 2: Correctly generate JSOP_FUNCALL and JSOP_FUNAPPLY. (r=arai) https://hg.mozilla.org/integration/mozilla-inbound/rev/8544285b4145 Part 3: Don't generate both JSOP_DEFVAR and JSOP_DEFFUN for toplevel functions. (r=arai) https://hg.mozilla.org/integration/mozilla-inbound/rev/275f7949148b Part 4: Respect strictness directives. (r=arai)
Backout by nbeleuzu@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/63ea63571271 Backed out 10 changesets (bug 1494930, bug 1459067, bug 1459555) for build bustages on JSScript.cpp. CLOSED TREE
Comment 10•6 years ago
|
||
Pushed by efaustbmo@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/90003e4c4f3a Part 1: Various BinAST parser correctness fixes. (r=arai) https://hg.mozilla.org/integration/mozilla-inbound/rev/9fc8180f3621 Part 2: Correctly generate JSOP_FUNCALL and JSOP_FUNAPPLY. (r=arai) https://hg.mozilla.org/integration/mozilla-inbound/rev/afa82a9888b4 Part 3: Don't generate both JSOP_DEFVAR and JSOP_DEFFUN for toplevel functions. (r=arai) https://hg.mozilla.org/integration/mozilla-inbound/rev/8820b0b524e0 Part 4: Respect strictness directives. (r=arai)
Comment 11•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/90003e4c4f3a https://hg.mozilla.org/mozilla-central/rev/9fc8180f3621 https://hg.mozilla.org/mozilla-central/rev/afa82a9888b4 https://hg.mozilla.org/mozilla-central/rev/8820b0b524e0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in
before you can comment on or make changes to this bug.
Description
•