Closed Bug 634800 Opened 13 years ago Closed 12 years ago

clean up reportCompileErrorNumberVA()

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Depends on: 638034
Attached patch patch (obsolete) — Splinter Review
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?
Attachment #623574 - Flags: review?(jwalden+bmo)
BTW, the patch is on top of the one in bug 634444.
Depends on: 634444
Attachment #623574 - Flags: review?(jwalden+bmo) → review+
And, yes, false means you stop doing what you were doing, because either OOM or an error happened.
Attached patch patch, v2Splinter Review
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.
Attachment #623574 - Attachment is obsolete: true
Attachment #633005 - Flags: review?(jwalden+bmo)
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.
Attachment #633005 - Flags: review?(jwalden+bmo) → 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.

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).
Well, at least I'm consistent.  :-)
https://hg.mozilla.org/mozilla-central/rev/67398aacf866
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.