735881 - StringBuffer::finishString doesn't always clear the buffer on success

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch and test

Specifically, it looks like at least if the short-string optimization applies, we don't clear the buffer out before returning.  I don't think we rely on this promise anywhere, so it's not important this get fixed immediately.  But docs shouldn't lie, so we should fix it.  And, come to think of it, it might not be a bad idea to use it in things like the JSON parser (which can create multiple string buffers but only ever uses one at a time) or other places where string buffers could be reused after emptying.

This better be the last StringBuffer issue I see in the near future.  :-)

Comment 1

[Luke Wagner [:luke]]

Since big string buffers are extracted from the Vector, I don't see this allowing optimization opportunities.  Thus, I'm not sure its such a good idea to make StringBuffer multi-shot.

Comment 2

[Jeff Walden [:Waldo]]

It avoids construction of the string buffer in the first place.  That's reasonably quick, sure, but it's not nothing.

Comment 3

[Luke Wagner [:luke]]

Construction is inline and maybe 4 stores to set up the Vector header.  I say single-shot and, if anything, assert no use-after-finish().

Comment 4

[Jeff Walden [:Waldo]]

You realize how many places would require assertions to do that, right?  That's every method on the entire thing, which means no obviously-simple forwarding for anything any more (begin, end, length, etc.).  It really is simplest to allow it to be reused.

Comment 5

[Luke Wagner [:luke]]

Just put it at the beginning of a focal point like finishString/finishAtom.  The difference between the two is, if you miss a clear(), its a bug.  If you miss an assert, nothing.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.