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

[Jim Gish]

Alan & Chris,

I agree with you that the new approach is less clear than the previous 
approach, but the original approach suffered from code duplication which 
was the motivation for the change.  However, let me propose something 
else.  How about /all /the methods in StringBuffer be synchronized?  
Although this is not strictly necessary, it works because reentrant 
synchronization is allowed. This eliminates the original problem with 
code duplication with the dispatch during the narrowing of types being 
done in both StringBuilder and StringBuffer, and also eliminates the 
confusion about where synchronization is being done and having to test 
for it.

I can still add a test to ensure that all methods of StringBuffer are 
synchronized, but that now becomes far easier - I simply can use 
reflection on each method and test for isSynchronized().  Having to do 
invocation tests to check for blocking or not seems much harder (unless 
you have a trick in your bag that I don't yet know).

Sound reasonable?

Thanks,
    Jim
On 09/30/2012 11:02 AM, Alan Bateman wrote:
> On 30/09/2012 09:27, Chris Hegarty wrote:
>> 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.
> I looked through the proposed changes and don't see anything obviously 
> wrong.
>
> I agree with Chris's comment that removing the explicit 
> synchronization from the methods in StringBuffer makes it less clear 
> that the methods actually do synchronize as specified. How about 
> adding a comment to make it clear that the synchronization is achieved 
> by calling into other StringBuffer methods? There was such a comment 
> in insert and lastIndexOf but they have been removed for some reason.
>
> On the tests then it would be really nice if there was a test that 
> verified that each one of the StringBuilder does synchronize as 
> specified. That would give people confidence when doing clean-ups like 
> this one. Also can you check the coverage of the insert methods? Since 
> you changed them too then we need to be sure that they are well tested.
>
> I'm in too minds on the addition of @Override, some people like it, 
> some people don't. With the proposed changes then you've added it to a 
> subset of the methods that are overridden so it's a bit inconsistent.
>
> -Alan.
>
>

-- 
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