Last Comment Bug 634800 - clean up reportCompileErrorNumberVA()
: clean up reportCompileErrorNumberVA()
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on: 634444 638034
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-16 18:57 PST by Nicholas Nethercote [:njn]
Modified: 2012-06-24 20:07 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.54 KB, patch)
2012-05-13 21:06 PDT, Nicholas Nethercote [:njn]
jwalden+bmo: review+
Details | Diff | Review
patch, v2 (7.99 KB, patch)
2012-06-13 20:55 PDT, Nicholas Nethercote [:njn]
jwalden+bmo: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] 2011-02-16 18:57:02 PST
reportCompileErrorNumberVA() is a mess, mostly due to the tortuous control flow, which means that most of the variables have to be declared at the top.  Surely it can be improved.
Comment 1 Nicholas Nethercote [:njn] 2012-05-13 21:06:45 PDT
Created attachment 623574 [details] [diff] [review]
patch

This does a bit of reordering, and moves some var decls away from the top.

One thing that puzzles me is the return value of this function.  It clearly returns |false| if it OOMs, and that seems to be how its callers are interpreting its return value.  But it also returns |false| if it reports an error (i.e. not a warning).  I guess in either of those two cases we want to immediately stop whatever scannning/parsing/emitting we're doing?
Comment 2 Nicholas Nethercote [:njn] 2012-05-13 21:07:25 PDT
BTW, the patch is on top of the one in bug 634444.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-15 14:57:16 PDT
And, yes, false means you stop doing what you were doing, because either OOM or an error happened.
Comment 4 Nicholas Nethercote [:njn] 2012-06-13 20:55:47 PDT
Created attachment 633005 [details] [diff] [review]
patch, v2

Here's a nicer version that uses RAII to avoid the goto statements.

I considered using ScopedFreePtr as well, but since the freeing of report.messageArgs is too complicated for that, I decided to do all the deallocations in a single class.
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-22 14:12:04 PDT
Comment on attachment 633005 [details] [diff] [review]
patch, v2

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

A little nicer, a little nicer.  Still a ways to go here regarding cleaning up all the API cruft here, tho; hopefully we can keep simplifying error reporting in the future.

::: js/src/frontend/TokenStream.cpp
@@ +449,5 @@
> +    ReportManager mgr(cx, &report, hasCharArgs);
> +
> +    if (!js_ExpandErrorArguments(cx, js_GetErrorMessage, NULL, errorNumber, &mgr.message, &report,
> +                                 hasCharArgs, ap))
> +        return false;

This needs bracing.

@@ +485,5 @@
>          JS_ASSERT(windowLength <= windowRadius * 2);
>  
>          // Create the windowed strings.
>          StringBuffer windowBuf(cx);
> +        if (!windowBuf.append(windowBase, windowLength) || !windowBuf.append((jschar)0))

Could you change that to '\0' while you're here?

@@ +524,5 @@
>           */
>          bool reportError = true;
>          if (JSDebugErrorHook hook = cx->runtime->debugHooks.debugErrorHook)
> +            reportError = hook(cx, mgr.message, &report,
> +                               cx->runtime->debugHooks.debugErrorHookData);

If it's two lines, braces.

::: js/src/frontend/TokenStream.h
@@ +500,5 @@
>      bool isEOF() const { return !!(flags & TSF_EOF); }
>      bool hasOctalCharacterEscape() const { return flags & TSF_OCTAL_CHAR; }
>  
> +    // Return false if we should stop compiling, either because (a) we issued
> +    // an error message, or (b) something went wrong, such as an OOM.

In principle there's nothing wrong with this comment.  But I can't help thinking that this behavior -- return-false implying an error, and the caller should propagate it -- is so idiomatic that this comment really isn't necessary.  Meh.
Comment 6 Nicholas Nethercote [:njn] 2012-06-22 19:12:28 PDT
> A little nicer, a little nicer.  Still a ways to go here regarding cleaning
> up all the API cruft here, tho; hopefully we can keep simplifying error
> reporting in the future.

Yes.  Unfortunately JSErrorReport is exposed in jsapi.h which makes it harder.


> > +        if (!windowBuf.append(windowBase, windowLength) || !windowBuf.append((jschar)0))
> 
> Could you change that to '\0' while you're here?

You asked me to do that in another review, but it causes ambiguous overloading of append() (see bug 634444 comment 45).
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-23 10:19:47 PDT
Well, at least I'm consistent.  :-)
Comment 8 Nicholas Nethercote [:njn] 2012-06-24 17:36:58 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/67398aacf866

Well that only took me 16 months!
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-06-24 20:07:46 PDT
https://hg.mozilla.org/mozilla-central/rev/67398aacf866

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