Closed
Bug 634800
Opened 13 years ago
Closed 12 years ago
clean up reportCompileErrorNumberVA()
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(1 file, 1 obsolete file)
7.99 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
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)
Assignee | ||
Comment 2•12 years ago
|
||
BTW, the patch is on top of the one in bug 634444.
Depends on: 634444
Updated•12 years ago
|
Attachment #623574 -
Flags: review?(jwalden+bmo) → review+
Comment 3•12 years ago
|
||
And, yes, false means you stop doing what you were doing, because either OOM or an error happened.
Assignee | ||
Comment 4•12 years ago
|
||
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 5•12 years ago
|
||
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+
Assignee | ||
Comment 6•12 years ago
|
||
> 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•12 years ago
|
||
Well, at least I'm consistent. :-)
Assignee | ||
Comment 8•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/67398aacf866 Well that only took me 16 months!
Comment 9•12 years ago
|
||
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.
Description
•