Updated webrev adapting feedback from Stuart. I need a sponsor to push the fix. Thanks for reviewing.
Cheers, Henry On 07/30/2013 11:27 AM, Henry Jen wrote: > > 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 >
