Closed
Bug 558446
Opened 13 years ago
Closed 12 years ago
CONCATN: toString methods called in wrong order
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jorendorff, Assigned: luke)
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file)
2.72 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
var s = ''; var a = {toString: function () { s += 'a'; return 'a'; }}; var b = {toString: function () { s += 'b'; return 'b'; }}; function f(a, b) { return a + " " + b; } f(a, b); assertEq(s, 'ab'); // fails: 'ba' The relevent bit of bytecode in f: getarg 0 string " " getarg 1 objtostr concatn 2 add I think ideally it should read: getarg 0 objtostr string " " getarg 1 objtostr concatn 3 but in any case, given a + "b" + c + d + e + f, a must be ToString-ed before the rest are evaluated. Luke, can you take this?
![]() |
Assignee | |
Comment 2•13 years ago
|
||
Should've caught this with bug 545165... The original strategy combined the results of the first N not-necessarily-string additions with concatn's results with an add which occurs after everything, thus the delayed side effect. Stringifying early and using concatn by itself both fixes the problem and removes an unnecessary temporary.
Attachment #438208 -
Flags: review?(jwalden+bmo)
![]() |
Assignee | |
Comment 3•13 years ago
|
||
Note to self: can remove JOF_TMPSLOT2 from JSOP_CONCATN OPDEF
Comment 4•13 years ago
|
||
Comment on attachment 438208 [details] [diff] [review] fix >diff --git a/js/src/jsemit.cpp b/js/src/jsemit.cpp >+ if (!pn2) >+ break; Nice early-exit here, we should have done this before. >+ /* >+ * Having seen a string literal, we know statically that the rest >+ * of the additions are string concatenation, so we emit them as a >+ * single concatn. First, do string conversion on the result of the >+ * preceding zero or more additions so that any side effects of >+ * string conversion occur before the next operand begins. >+ */ >+ index = 0; >+ if (pn2 != pn->pn_head) { >+ if (!js_Emit1(cx, cg, JSOP_OBJTOSTR)) >+ return JS_FALSE; >+ index++; >+ } >+ Suggest pushing |index = 1| down below this block -- zeroing then always incrementing on success doesn't seem to have much purpose. You can also convert the check-and-emit-maybe-fail bits into two lines rather than four. >+ for (; pn2; pn2 = pn2->pn_next, index++) { > if (!js_EmitTree(cx, cg, pn2)) > return JS_FALSE; >+ if (!pn2->isLiteral() && js_Emit1(cx, cg, JSOP_OBJTOSTR) < 0) >+ return JS_FALSE; >+ } do { if (!js_EmitTree(...) ...; if (!pn2->isLiteral() && ...) ...; index++; } while ((pn2 = pn2->pn_next) != NULL);
Attachment #438208 -
Flags: review?(jwalden+bmo) → review+
![]() |
Assignee | |
Comment 5•13 years ago
|
||
(In reply to comment #4) > >+ for (; pn2; pn2 = pn2->pn_next, index++) { > > if (!js_EmitTree(cx, cg, pn2)) > > return JS_FALSE; > >+ if (!pn2->isLiteral() && js_Emit1(cx, cg, JSOP_OBJTOSTR) < 0) > >+ return JS_FALSE; > >+ } > > do { > if (!js_EmitTree(...) ...; > if (!pn2->isLiteral() && ...) ...; > index++; > } while ((pn2 = pn2->pn_next) != NULL); As a general rule, I prefer to increment loop variables, like index, in the increment clause of for loops. It conveys the intention clearly (both to the reader and compiler). I think this clarity is worth the single unnecessary branch.
![]() |
Assignee | |
Comment 6•13 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/47d29a5adb38
Whiteboard: fixed-in-tracemonkey
Comment 7•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/47d29a5adb38
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•