CFR: 8020977: StringJoiner merges with itself not as expected
henry.jen at oracle.com
Tue Jul 30 18:27:16 UTC 2013
On Jul 30, 2013, at 10:00 AM, Stuart Marks <stuart.marks at oracle.com> wrote:
> 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.
That's fine, I think the reason in earlier version to capture the other.value is to avoid member access multiple times?
As you said, I only try to get the length as that's enough to ensure we "seize" the data.
> 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.
Your analysis is correct, and I think it's really just defensive. I don't think it can happen as we do guard against other.value == null, which is the only case length will be less than prefix.
More information about the core-libs-dev