CFR: 8020977: StringJoiner merges with itself not as expected

[Stuart Marks]

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 
data".

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.

Thanks,

s'marks




On 7/29/13 7:43 PM, Henry Jen wrote:
> Sorry, the webrev is at
>
> http://cr.openjdk.java.net/~henryjen/tl/8020977/0/webrev/
>
> 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
>>
>