On Jul 30, 2013, at 10:00 AM, Stuart Marks <stuart.ma...@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. Cheers, Henry
