Code Review Request for Bug #5035850

[Stuart Marks]

Hi Darryl,

I don't want to nickel-and-dime you here but there are a few small things I 
wanted to mention.

  - The test is really about String.CaseInsensitiveComparator.readResolve, not 
String.readResolve. The comments at the top of the test should be updated.

  - Extra semicolon at line 50.

(The following is mainly directed at Mike and David.)

I think it's confusing to have the tests for equals() when the implementation 
today doesn't override equals(). Even if an equals() override were added in the 
future, this test would have to be rewritten to allow for instances that were 
equals() but != , so having the equals() checks don't provide any 
future-proofing either.

s'marks

On 11/23/11 10: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
>
> 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
>>>>>
>>>>>