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.g...@oracle.com
