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
