Closed Bug 889584 Opened 11 years ago Closed 11 years ago

Four trivial parser cleanups

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

Attachments

(4 files)

      No description provided.
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)
A bad comment is better than none? This is all I know. More detailed info welcome.
Attachment #770428 - Flags: review?(jwalden+bmo)
The two specializations are basically the same code; no point keeping two copies.
Attachment #770429 - Flags: review?(jwalden+bmo)
Attachment #770429 - Attachment description: Un-specialize Parser::expr() → Part 3 - Un-specialize Parser::expr()
FullParseHandler::newFunctionDefinition() already initializes that field to NULL. And SyntaxParseHandler::setFunctionBody() is a no-op.
Attachment #770431 - Flags: review?(jwalden+bmo)
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 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 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+
Attachment #770431 - Flags: review?(jwalden+bmo) → review+
(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.

Attachment

General

Created:
Updated:
Size: