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.
