Last Comment Bug 754739 - Clean up front-end error reporting
: Clean up front-end error reporting
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on: 754181
Blocks: UntangleFrontEnd
  Show dependency treegraph
 
Reported: 2012-05-13 18:17 PDT by Nicholas Nethercote [:njn]
Modified: 2012-06-26 01:59 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (108.42 KB, patch)
2012-05-13 18:17 PDT, Nicholas Nethercote [:njn]
no flags Details | Diff | Splinter Review
rebased patch (112.23 KB, patch)
2012-05-24 17:30 PDT, Nicholas Nethercote [:njn]
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2012-05-13 18:17:00 PDT
Created attachment 623544 [details] [diff] [review]
patch

This patch cleans up error reporting in the front-end.

It introduces error reporters that are specific to classes:

  TokenStream::report{Error,Warning,StrictWarning,StrictModeError}
  Parser::report{Error,UcError,Warning,StrictWarning,StrictModeError}
  BytecodeEmitter::report{Error,StrictWarning,StrictModeError}

(We already had Parser::reportErrorNumber, that's been removed in favour of
Parser::reportError.)  Note that the TokenStream ones don't take a 
ParseNode* because they don't need to.

These call the existing general-purpose
TokenStream::reportCompileErrorNumberVA, some of them via the new
Tokenstream::reportStrictModeErrorNumberVA function.

There's a bit of repetition in the definition of these new reportFoo() 
functions, mostly due to the awkwardness of handling varargs.  I think
that's worth it to make the hundreds of callsites nicer.  

The presence of these new functions meant I could remove 
ReportCompileErrorNumber and ReportStrictModeError.
Comment 1 Nicholas Nethercote [:njn] 2012-05-24 17:30:21 PDT
Created attachment 627046 [details] [diff] [review]
rebased patch

Just rebased for recent changes.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-24 17:45:46 PDT
Without having even looked at the patch, I am going to preemptively claim to be in favor of this.  \o/
Comment 3 Nicholas Nethercote [:njn] 2012-06-08 00:19:48 PDT
Comment on attachment 627046 [details] [diff] [review]
rebased patch

Sorry to do this, Waldo, but jimb is going on vacation and won't get to this for at least another 3 weeks.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-22 13:43:13 PDT
Comment on attachment 627046 [details] [diff] [review]
rebased patch

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

::: js/src/frontend/BytecodeEmitter.h
@@ +194,5 @@
>      unsigned currentLine() const { return current->currentLine; }
>  
>      inline ptrdiff_t countFinalSourceNotes();
> +
> +    bool reportError(ParseNode *pn, unsigned errorNumber, ...);  // uses JSREPORT_ERROR

This comment seems unnecessary to me.

::: js/src/frontend/Parser.cpp
@@ +1947,3 @@
>      {
>          return NULL;
>      }

Unbrace.

@@ +2654,1 @@
>                            (tt == TOK_RETURN) ? js_return_str : js_yield_str);

Indentation.

::: js/src/frontend/Parser.h
@@ +291,5 @@
> +
> +inline bool
> +Parser::reportStrictWarning(ParseNode *pn, unsigned errorNumber, ...)
> +{
> +    va_list ap;

Either name 'em all args, or name 'em all ap, whichever prevails.

::: js/src/frontend/TokenStream.cpp
@@ +1291,1 @@
>      }

Unbrace.

::: js/src/frontend/TokenStream.h
@@ +511,5 @@
> +    // General-purpose error reporters, used by TokenStream, Parser, and
> +    // BytecodeEmitter.
> +    bool reportCompileErrorNumberVA(ParseNode *pn, unsigned flags, unsigned errorNumber,
> +                                    va_list ap);
> +    bool reportStrictModeErrorNumberVA(ParseNode *pn, unsigned errorNumber, va_list ap);

Any chance you could make these private and make the methods that need to call them friends, as there are now much-better-signatured methods to use instead?

::: js/src/jsfun.cpp
@@ +1120,4 @@
>                              return false;
>                          }
>                      }
>                      else {

While you're in the area could you fix this bracing?

::: js/src/jsxml.cpp
@@ +1537,1 @@
>                      }

Unbrace.

@@ +1579,1 @@
>                  }

Unbrace.
Comment 5 Nicholas Nethercote [:njn] 2012-06-24 23:06:51 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/611574c8fc1e


> ::: js/src/frontend/TokenStream.h
> @@ +511,5 @@
> > +    // General-purpose error reporters, used by TokenStream, Parser, and
> > +    // BytecodeEmitter.
> > +    bool reportCompileErrorNumberVA(ParseNode *pn, unsigned flags, unsigned errorNumber,
> > +                                    va_list ap);
> > +    bool reportStrictModeErrorNumberVA(ParseNode *pn, unsigned errorNumber, va_list ap);
> 
> Any chance you could make these private and make the methods that need to
> call them friends, as there are now much-better-signatured methods to use
> instead?

That would have required #include Parser.h and BytecodeEmitter.h into TokenStream.h, which would have created cyclic header dependencies :(
Comment 6 Ed Morley [:emorley] 2012-06-26 01:59:43 PDT
https://hg.mozilla.org/mozilla-central/rev/611574c8fc1e

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