Code Review Request for Bug #5035850

Darryl Mocek darryl.mocek at
Tue Nov 22 18:32:14 UTC 2011

I've resolved all issues given in the comments.  Please review the 
update.  Webrev can be found here:


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