The default bug view has changed. See this FAQ.

Clean up front-end error reporting

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
Attachment #623544 - Flags: review?(jimb)
Whiteboard: [js:t]
(Assignee)

Comment 1

5 years ago
Created attachment 627046 [details] [diff] [review]
rebased patch

Just rebased for recent changes.
Attachment #623544 - Attachment is obsolete: true
Attachment #623544 - Flags: review?(jimb)
Attachment #627046 - Flags: review?(jimb)
Without having even looked at the patch, I am going to preemptively claim to be in favor of this.  \o/
(Assignee)

Comment 3

5 years ago
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.
Attachment #627046 - Flags: review?(jimb) → review?(jwalden+bmo)
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.
Attachment #627046 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 5

5 years ago
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 :(
https://hg.mozilla.org/mozilla-central/rev/611574c8fc1e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.