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

[Alan Bateman]

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.