754739 - Clean up front-end error reporting

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: 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 [inactive]]

Attachment: rebased patch

Just rebased for recent changes.

Comment 2

[Jeff Walden [:Waldo]]

Without having even looked at the patch, I am going to preemptively claim to be in favor of this.  \o/

Comment 3

[Nicholas Nethercote [inactive]]

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]]

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 [inactive]]

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]]

https://hg.mozilla.org/mozilla-central/rev/611574c8fc1e