Closed Bug 1045848 Opened 6 years ago Closed 6 years ago

MUST_MATCH_TOKEN should be a method, not report error when token is TOK_ERROR

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: gupta.rajagopal, Assigned: gupta.rajagopal)

Details

Attachments

(1 file, 3 obsolete files)

1. MUST_MATCH_TOKEN should be a method rather than a macro
2. It should not call report() when the token obtained is TOK_ERROR because this just replaces a more useful error message with a generic error message
3. The comment about cx and ts is not valid anymore.
Assignee: nobody → gupta.rajagopal
Status: NEW → ASSIGNED
Attachment #8468606 - Flags: review?(jorendorff)
Comment on attachment 8468606 [details] [diff] [review]
Patch to replace MUST_MATCH_TOKEN v1

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

Thanks for doing this! I just have one nit about comments. r=me with that.

::: js/src/frontend/Parser.cpp
@@ +61,1 @@
>   */

Please use this instead:

/* Read a token. Report an error and return false if that token isn't of type tt. */

I don't think it helps to try to explain in any more detail here.

@@ +65,5 @@
> +{
> +    TokenKind token = tokenStream.getToken();
> +    if (token != tt) {
> +        if (token != TOK_ERROR)
> +            report(ParseError, false, null(), errno);

If you'd like to explain more about TOK_ERROR, this is the place to do it -- using //-style comments. It's really an implementation detail of this method, not part of the contract. No caller cares about that detail.
Attachment #8468606 - Flags: review?(jorendorff) → review+
Fixed nit.
Attachment #8468606 - Attachment is obsolete: true
Attachment #8469553 - Flags: review+
Compilation fails on XP with the previous patch.

In this new patch, MUST_MATCH_TOKEN remains a macro. The functionality is corrected, though.
Attachment #8469553 - Attachment is obsolete: true
Attachment #8470247 - Flags: review?(jorendorff)
Comment on attachment 8470247 [details] [diff] [review]
Patch that corrects MUST_MATCH_TOKEN v4

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

::: js/src/frontend/Parser.cpp
@@ +55,5 @@
>  typedef Rooted<NestedScopeObject*> RootedNestedScopeObject;
>  typedef Handle<NestedScopeObject*> HandleNestedScopeObject;
>  
>  
> +/* Read a token. Report an error and return false if that token isn't of type tt. */

Since it's a macro again, it returns null(), not false.
Attachment #8470247 - Flags: review?(jorendorff) → review+
Corrected comment.
Attachment #8470247 - Attachment is obsolete: true
Attachment #8472496 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/40aad048fc17
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.