Code Review Request for Bug #5035850

[David Holmes]

On 24/11/2011 10:15 AM, Stuart Marks wrote:
> 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.

As I said it protects against a future erroneous definition of equals(). 
In this case an equals() that was not consistent with == would be an 
erroneous definition. But the only reason to define equals() would be if 
this were no longer a singleton class, in which case the test would need 
to be updated anyway.

I too am trying not to nickel-and-dime with people's suggestions, but 
these quick fixes do tend to explode somewhat once a number of pairs of 
eyes are directed to them.

David
-----

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