Re: CFR: 8020977: StringJoiner merges with itself not as expected

2013-07-30 Thread Henry Jen
My bad, you should be on the reviewer list. After all, this update is based on your feedback. Webrev updated with Reviewed-by. Cheers, Henry On Jul 30, 2013, at 3:03 PM, Stuart Marks wrote: > On 7/30/13 1:59 PM, Henry Jen wrote: >> On 07/30/2013 01:58 PM, Henry Jen wrote: >>> Updated webrev

Re: CFR: 8020977: StringJoiner merges with itself not as expected

2013-07-30 Thread Stuart Marks
On 7/30/13 1:59 PM, Henry Jen wrote: On 07/30/2013 01:58 PM, Henry Jen wrote: Updated webrev adapting feedback from Stuart. I need a sponsor to push the fix. Thanks for reviewing. Sigh, again, the webrev is at http://cr.openjdk.java.net/~henryjen/tl/8020977/1/webrev/ Looks good, thanks for

Re: CFR: 8020977: StringJoiner merges with itself not as expected

2013-07-30 Thread Henry Jen
On 07/30/2013 01:58 PM, Henry Jen wrote: > Updated webrev adapting feedback from Stuart. > > I need a sponsor to push the fix. Thanks for reviewing. > Sigh, again, the webrev is at http://cr.openjdk.java.net/~henryjen/tl/8020977/1/webrev/ Cheers, Henry

Re: CFR: 8020977: StringJoiner merges with itself not as expected

2013-07-30 Thread Henry Jen
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 wrote: > >> Hi Henry, >> >> A couple minor comments on this. >> >> I don't think it's ne

Re: CFR: 8020977: StringJoiner merges with itself not as expected

2013-07-30 Thread Henry Jen
On Jul 30, 2013, at 10:00 AM, Stuart Marks 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 th

Re: CFR: 8020977: StringJoiner merges with itself not as expected

2013-07-30 Thread 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 b

Re: CFR: 8020977: StringJoiner merges with itself not as expected

2013-07-30 Thread Mike Duigou
Looks good to me as well. Mike On Jul 29 2013, at 19:30 , 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

Re: CFR: 8020977: StringJoiner merges with itself not as expected

2013-07-30 Thread Chris Hegarty
On 07/30/2013 12:02 PM, Paul Sandoz wrote: On Jul 30, 2013, at 3:43 AM, Henry Jen wrote: Sorry, the webrev is at http://cr.openjdk.java.net/~henryjen/tl/8020977/0/webrev/ Looks ok to me. +1 looks good to me too. -Chris. Paul.

Re: CFR: 8020977: StringJoiner merges with itself not as expected

2013-07-30 Thread Paul Sandoz
On Jul 30, 2013, at 3:43 AM, Henry Jen wrote: > Sorry, the webrev is at > > http://cr.openjdk.java.net/~henryjen/tl/8020977/0/webrev/ > Looks ok to me. Paul.

Re: CFR: 8020977: StringJoiner merges with itself not as expected

2013-07-29 Thread Henry Jen
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

CFR: 8020977: StringJoiner merges with itself not as expected

2013-07-29 Thread Henry Jen
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