CFR: 8020977: StringJoiner merges with itself not as expected

Stuart Marks stuart.marks at
Tue Jul 30 17:00:54 UTC 2013

Hi Henry,

A couple minor comments on this.

I don't think it's necessary to snapshot other.value into a local otherBuilder 
variable. If other == this, when the value is mutated by prepareBuilder(), 
otherBuilder still points to this.value, whose contents have changed. So the 
actual data isn't being snapshotted, as is implied by the comment "seize the 

The essence of the change is to snapshot other's original length. Of course 
this works because we know that prepareBuilder() only appends, so we don't have 
to copy the entire value.

I'd just use other.value and clarify the comment.


It's a separate issue but I spent some time puzzling over the 
(other.prefix.length() < length) condition trying to figure out why it's 
necessary. It turns out that it isn't. :-) [At least that I could see; the 
tests may prove me wrong.]

It's invariant that other.prefix.length() <= other.value.length(). They're only 
equal length if somebody has added the empty string to other. So the condition 
saves a call to append() if this is the case. Having this test is an 
optimization of sorts, but append() handles appending a zero-length interval 
just fine, so IMO this extra test adds a bit of clutter to the code.

Up to you whether you want to do anything about this.



On 7/29/13 7:43 PM, Henry Jen wrote:
> Sorry, the webrev is at
> Cheers,
> Henry
> On 07/29/2013 07:30 PM, Henry Jen wrote:
>> Hi,
>> Please review a simple fix for 8020977.
>> The fix grab the length before initiate copying operation, so that the
>> 'delimiter' added for this won't be included in copy.
>> For rest cases, the timing window changed a little, but that's fine as
>> simultaneous changes is not determined.
>> Cheers,
>> Henry

More information about the core-libs-dev mailing list