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

ASSIGNED
Assigned to

Status

()

--
minor
ASSIGNED
7 years ago
6 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

7 years ago
Created attachment 605987 [details] [diff] [review]
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.  :-)
Attachment #605987 - Flags: review?(luke)

Comment 1

7 years ago
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.
(Assignee)

Comment 2

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

Comment 3

7 years ago
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().
(Assignee)

Comment 4

7 years ago
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

7 years ago
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.

Updated

6 years ago
Attachment #605987 - Flags: review?(luke) → review-
You need to log in before you can comment on or make changes to this bug.