RFR: 6206780 (str) Forwarding append methods in String{Buffer, Builder} are inconsistent

[Chris Hegarty]

Jim,

I can confirm that the changes in the updated webrev are as per my 
comments, and that behavior should be maintained.

I have no problem with the changes in ABS and builder, but I'm not so 
sure about the changes in buffer. With delegation to the super class it 
is not clear where the synchronization is actually happening (at least 
not to me).

The old direct delegation to other sync'ed methods in its own class is 
very understandable. I know, my last comment was to remove the explicit 
sycn blocks because they are unnecessary, but now I'm thinking that the 
old/original code here is actually more readable/understandable, with 
respect to where the synchronization is happening.

Maybe others would have an opinion on this.

-Chris.

On 28/09/12 19:09, Jim Gish wrote:
> Please review the changes described below.  Again, at
> http://cr.openjdk.java.net/~jgish/Bug6206780-sb-append-forward/
> <http://cr.openjdk.java.net/%7Ejgish/Bug6206780-sb-append-forward/>
>
> Thanks,
>     Jim
>
> On 09/26/2012 01:22 PM, Jim Gish wrote:
>>
>> On 09/26/2012 06:12 AM, Chris Hegarty wrote:
>>> Hi Jim,
>>>
>>> This is a nice cleanup, and certainly makes builder and buffer more
>>> consistent.
>>>
>>> Some specific comments:
>>>  - StringBuilder
>>>    Why the change to the public doc for append(StringBuffer)? It
>>>    looks wrong to me. It is now taking about the wrong given type. \
>> accident - fixed.
>>>
>>>  - Trivially, the year in the header of the tests is incorrect
>> Fixed
>>>
>>>  - StringBuffer
>>>    I don't see that any of the new synchronized blocks are necessary.
>>>    If ASB is doing exactly the same as the previous buffer code, then
>>>    it will invoke the overridden subclass methods that are already
>>>    synchronized.
>>>
>> Good catch. I verified this, and you're absolutely right.  Fixed.
>>
>> Thanks.  I'll re-spin the patch and send it your way for pushing, if
>> you would be so kind, please.
>>
>> Cheers,
>>    Jim
>>
>>> -Chris.
>>>
>>> On 25/09/2012 23:03, Jim Gish wrote:
>>>> Please review the change at
>>>> http://cr.openjdk.java.net/~jgish/Bug6206780-sb-append-forward
>>>>
>>>> Overview --
>>>>
>>>> * introduced consistent forwarding to AbstractStringBuilder from
>>>> StringBuffer and StringBuilder for append and other methods per the
>>>> bug report.
>>>> * Added missing @Override annotations.
>>>> * Got rid of knowledge of the sub-classes from ASB (smelled too bad to
>>>> leave in).
>>>> * Retained all methods to maintain compatibility (even though some
>>>> like append(StringBuffer) could go away because append(CharSequence)
>>>> would do).
>>>> * To further maintain compatibility, used sychronized(this) in place
>>>> of adding synchronized methods to those methods in StringBuffer that
>>>> now need it (because of type-based dispatch being done in super
>>>> rather than in /both/ StringBuffer and StringBuilder with
>>>> CharSequence args).
>>>> * Ensured that StringBuffer.append(StringBuilder) and
>>>> StringBuilder.append(StringBuffer) are both handled properly.
>>>>
>>>>
>>>> Thanks,
>>>> Jim
>>>>
>>
>
> --
> Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
> Oracle Java Platform Group | Core Libraries Team
> 35 Network Drive
> Burlington, MA 01803
> jim.gish at oracle.com
>