Closed Bug 789239 Opened 7 years ago Closed 7 years ago

Unravel 30-line if condition in Parser.cpp

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

(Whiteboard: [js:t])

Attachments

(2 files)

Because yeah.
Assignee: general → jorendorff
Attachment #658982 - Flags: review?(jwalden+bmo)
Bless you, my son.
Comment on attachment 658985 [details] [diff] [review]
Part 2 - Rewrite the function using early returns

Review of attachment 658985 [details] [diff] [review]:
-----------------------------------------------------------------

<applause>
Whiteboard: [js:t]
Comment on attachment 658982 [details] [diff] [review]
Part 1 - Move the condition into a helper function

Review of attachment 658982 [details] [diff] [review]:
-----------------------------------------------------------------

This patch seems to be a half-amalgam of the change here and the changes in bug 728079, which confused me a bit.

::: js/src/jit-test/tests/for-of/bug-728079-js17-3.js
@@ +1,3 @@
> +// Cleaned-up version of bug 728079 comment 0.
> +
> +version(170);

I assume this belonged in bug 728079's patch?
Attachment #658982 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 658985 [details] [diff] [review]
Part 2 - Rewrite the function using early returns

Review of attachment 658985 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/Parser.cpp
@@ +2959,5 @@
>  {
> +    if (forDecl) {
> +        if (pn1->pn_count > 1)
> +            return false;
> +        if (pn1->isOp(JSOP_DEFCONST))

Probably shouldn't be in this change, just for more sequence points for bisection, but this can be isKind(PNK_CONST) now, I think.
Attachment #658985 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/2085233469a0
https://hg.mozilla.org/mozilla-central/rev/2e9d543ae2ec
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.