Code Review Request for Bug #5035850

[Darryl Mocek]

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/

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
>>
>>