Four trivial parser cleanups

RESOLVED FIXED in mozilla25

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

unspecified
mozilla25
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 770427 [details] [diff] [review]
Part 1 - Put all Parser::statement() parsing code in the switch statement, and make each case return

As discussed, leaving the semicolon code after the switch statement is lame; let's get rid of that, even if it means adding a /* FALL THROUGH */. Now each kind of statement has its own dedicated parsing code that parses the whole thing.
Assignee: general → jorendorff
Attachment #770427 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 2

5 years ago
Created attachment 770428 [details] [diff] [review]
Part 2 - comment withStatement()

A bad comment is better than none? This is all I know. More detailed info welcome.
Attachment #770428 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

5 years ago
Created attachment 770429 [details] [diff] [review]
Part 3 - Un-specialize Parser::expr()

The two specializations are basically the same code; no point keeping two copies.
Attachment #770429 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

5 years ago
Attachment #770429 - Attachment description: Un-specialize Parser::expr() → Part 3 - Un-specialize Parser::expr()
(Assignee)

Comment 4

5 years ago
Created attachment 770431 [details] [diff] [review]
Part 4 - Remove useless setFunctionBody() call

FullParseHandler::newFunctionDefinition() already initializes that field to NULL. And SyntaxParseHandler::setFunctionBody() is a no-op.
Attachment #770431 - Flags: review?(jwalden+bmo)

Comment 5

5 years ago
Comment on attachment 770427 [details] [diff] [review]
Part 1 - Put all Parser::statement() parsing code in the switch statement, and make each case return

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

Glorious.
Attachment #770427 - Flags: review?(jwalden+bmo) → review+

Comment 6

5 years ago
Comment on attachment 770428 [details] [diff] [review]
Part 2 - comment withStatement()

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

::: js/src/frontend/Parser.cpp
@@ +4460,5 @@
>  ParseNode *
>  Parser<FullParseHandler>::withStatement()
>  {
> +    // test262/ch12/12.10/12.10-0-1.js fails if we try to parse with-statements
> +    // in syntax-parse mode.

File a bug on this, note it here?
Attachment #770428 - Flags: review?(jwalden+bmo) → review+

Comment 7

5 years ago
Comment on attachment 770429 [details] [diff] [review]
Part 3 - Un-specialize Parser::expr()

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

Most excellent.

::: js/src/frontend/Parser.cpp
@@ +4858,2 @@
>      if (pn && tokenStream.matchToken(TOK_COMMA)) {
> +        Node seq = handler.newList(PNK_COMMA, pn);

Don't you need |if (!seq) return null();|?
Attachment #770429 - Flags: review?(jwalden+bmo) → review+

Updated

5 years ago
Attachment #770431 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 8

5 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> > +    // test262/ch12/12.10/12.10-0-1.js fails if we try to parse with-statements
> > +    // in syntax-parse mode.
> 
> File a bug on this, note it here?

-    // in syntax-parse mode.
+    // in syntax-parse mode. See bug 892583.

> Don't you need |if (!seq) return null();|?

Quite. Thanks.
You need to log in before you can comment on or make changes to this bug.