Code Review Request for Bug #5035850

David Holmes david.holmes at
Thu Nov 24 02:07:25 UTC 2011

On 24/11/2011 10:15 AM, Stuart Marks wrote:
> I don't want to nickel-and-dime you here but there are a few small
> things I wanted to mention.
> - The test is really about String.CaseInsensitiveComparator.readResolve,
> not String.readResolve. The comments at the top of the test should be
> updated.
> - Extra semicolon at line 50.
> (The following is mainly directed at Mike and David.)
> I think it's confusing to have the tests for equals() when the
> implementation today doesn't override equals(). Even if an equals()
> override were added in the future, this test would have to be rewritten
> to allow for instances that were equals() but != , so having the
> equals() checks don't provide any future-proofing either.

As I said it protects against a future erroneous definition of equals(). 
In this case an equals() that was not consistent with == would be an 
erroneous definition. But the only reason to define equals() would be if 
this were no longer a singleton class, in which case the test would need 
to be updated anyway.

I too am trying not to nickel-and-dime with people's suggestions, but 
these quick fixes do tend to explode somewhat once a number of pairs of 
eyes are directed to them.


> s'marks
> On 11/23/11 10:21 AM, Darryl Mocek wrote:
>> I've gone back to the try-with-resources implementation in the test
>> (which will
>> close resources), but specifically closing the ObjectOutputStream to
>> ensure
>> flushing of the data. I've also kept the three equals tests and fixed the
>> wording in the Exceptions.
>> Please review:
>> Thanks,
>> Darryl
>> On Tue 22 Nov 2011 10:39:01 PM PST, David Holmes wrote:
>>> Hi Darryl,
>>> On 23/11/2011 4:32 AM, Darryl Mocek wrote:
>>>> I've resolved all issues given in the comments. Please review the
>>>> update. Webrev can be found here:
>>> Minor nit: in the test you took Mike's suggestion for the three
>>> conditions to
>>> check but seem to have overlooked the slightly different wording Mike
>>> used in
>>> the error messages and have associated the messages with different
>>> conditions
>>> ie "match" vs "equals"
>>> Also wondering if we need to be concerned with closing the streams in
>>> case
>>> the test runs in samevm or agentvm ? Maybe use try-with-resources.
>>> Thanks,
>>> David
>>>> Thanks,
>>>> Darryl
>>>> On 11/20/2011 05:31 PM, David Holmes wrote:
>>>>> On 21/11/2011 5:39 AM, Rémi Forax wrote:
>>>>>> On 11/20/2011 08:14 PM, Alan Bateman wrote:
>>>>>>> On 18/11/2011 18:27, Darryl Mocek wrote:
>>>>>>>> Hello. Please review this patch to fix a serialization issue with
>>>>>>>> String's CASE_INSENSITIVE_ORDER. If you serialize, then deserialize
>>>>>>>> the class, the equals test will fail in the comparison of what was
>>>>>>>> serialized with what was deserialized. Webrev, including test,
>>>>>>>> can be
>>>>>>>> found here:
>>>>>>>> Thanks,
>>>>>>>> Darryl
>>>>>>> This looks okay to me but I would suggest adding a comment to
>>>>>>> readResolve, maybe something like "Replaces the de-serialized
>>>>>>> object"
>>>>>>> as the causal reader may not know what this method is for.
>>>>>>> -Alan.
>>>>>> Hi Darryl, Hi Alan,
>>>>>> additional comments: in the test, you don't need to initialize
>>>>>> result to
>>>>>> null because you can remove the catch(Exception) block
>>>>> Related to this I was going to say that if you get an unexpected
>>>>> exception I believe you should simply let it propagate to indicate
>>>>> failure.
>>>>>> and also you should use == instead of equals for the last check.
>>>>> Given equals() is not overridden the two are equivalent. But testing
>>>>> both as Mike suggests would catch any erroneous redefinition of
>>>>> equals() in the future.
>>>>> David
>>>>> -----
>>>>>> cheers,
>>>>>> Rémi

More information about the core-libs-dev mailing list