Code Review Request for Bug #5035850

[David Holmes]

On 24/11/2011 4: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: http://cr.openjdk.java.net/~dmocek/5035850/webrev.01

Looks okay to me except the test comments as Stuart stated.

Really this fix is to ensure that the singleton property of 
CaseInsensitiveComparator is maintained through 
serialization/deserialization.

David

> 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:
>>> http://cr.openjdk.java.net/~dmocek/5035850/
>>
>> 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:
>>>>>>> http://cr.openjdk.java.net/~sherman/darryl/5035850/webrev
>>>>>>>
>>>>>>> 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
>>>>>
>>>>>