Closed
Bug 724586
Opened 12 years ago
Closed 12 years ago
Replace raw strcats with StringBuffer in obj_toSource
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: terrence, Assigned: terrence)
References
Details
Attachments
(1 file, 1 obsolete file)
14.01 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
Also rips out some dangling sharp references and sanifies the control flow a bit. Allows us to rip out about 75 lines of fiddly C string manipulation. There is still more to do here before this code path is sane enough to replace the hash table.
Attachment #594741 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 1•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=12745cb4ba65
Comment 2•12 years ago
|
||
Comment on attachment 594741 [details] [diff] [review] v1 Review of attachment 594741 [details] [diff] [review]: ----------------------------------------------------------------- The extra changes I suggest probably orphan a few variables -- make sure to check for unused warnings before committing a patch with changes. ::: js/src/jsobj.cpp @@ +639,5 @@ > vchars = start; > } > } > > + if (comma && !(buf.append(comma[0]) && buf.append(comma[1]))) if (comma && !buf.append(comma, 2)) goto overflow; Or, since I think you said comma's either NULL or ", ", make comma a bool, then buf.append(", ") -- much clearer. @@ +650,5 @@ > goto overflow; > + if (!buf.append(gsopchars, gsop[j]->length())) > + goto overflow; > + if (!buf.append(' ')) > + goto overflow; if (!buf.append(gsop[j]) || !buf.append(' ')) goto overflow; Goodbye raw char* references! @@ +655,4 @@ > } > + > + if (!buf.append(idstrchars, idstrlength)) > + goto overflow; if (!buf.append(idstr)) goto overflow; @@ +673,5 @@ > + > + str = buf.finishString(); > + if (!str) { > + JS_ReportOutOfMemory(cx); > + return false; finishString would have reported an error if it failed, so this extra report is unnecessary (bad, even, see below). @@ +685,4 @@ > > overflow: > + js_LeaveSharpObject(cx, &ida); > + JS_ReportOutOfMemory(cx); If we can make all goto-overflows be the result of append failures, then Vector::append (whichever overload) would have reported an error, so this is unnecessary (bad, even, since we have assertions that errors aren't multiply-reported). And at that point error/overflow are identical, so they can be unified. And at that point, we can just make the js_LeaveSharpObject happen through a one-off RAII object. I don't think it matters if we leave before finishing the string and setting it in the success case. And at that point, all the error-y gotos become return false! And at that point we declare victory!
Attachment #594741 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 3•12 years ago
|
||
Worked like a charm. Rerunning try to ensure I didn't break anything. https://tbpl.mozilla.org/?tree=Try&rev=3eb0895f9745
Attachment #594741 -
Attachment is obsolete: true
Attachment #594889 -
Flags: review+
Assignee | ||
Comment 4•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/224e1e3e1483
Comment 5•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/224e1e3e1483
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in
before you can comment on or make changes to this bug.
Description
•