Code Review Request for Bug #5035850
darryl.mocek at oracle.com
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: http://cr.openjdk.java.net/~sherman/darryl/5035850/webrev
>>> 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.
>> 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
>> 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.
More information about the core-libs-dev