Code Review Request for Bug #5035850

[Mike Duigou]

On Nov 20 2011, at 11:39 , 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
> and also you should use == instead of equals for the last check.

I would actually use both to make sure that the equals implementation works correctly. Better yet,

if (!String.CASE_INSENSITIVE_ORDER.equals(result)) {
               throw new Exception("Value restored from serial form does not equal original");
           }

if (!result.equals(String.CASE_INSENSITIVE_ORDER)) {
               throw new Exception("Value restored from serial form does not equal original");
           }

if (String.CASE_INSENSITIVE_ORDER != result) {
               throw new Exception("Value restored from serial form does not match original!");
           }


I would also add an 'out.close();' after the writeObject to make sure everything is flushed to the ByteArrayOutputStream.

Mike