On 6/24/19, 2:23 AM, Daniel Fuchs wrote:
On 21/06/2019 21:20, Joe Wang wrote:
Full webrev (for the record)
http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev_02/
A short version of webrev_02 that includes the only files mentioned
in this review:
http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev_02_short/
Thanks Joe!
The updated files look good.
Thumbs up!
Thanks!
Minor comment - that you might want to propagate upstream:
The hashCode impl is indeed inconsistent with equals. Looking at the
issue [1], by using Objects.equals, they were fixing a possible NPE.
Either case (the original or the use of Objects.equals) does not affect
java.xml's specific usage of BCEL. The original JAXP team used to be
part of Apache projects, but I myself hasn't been. While we would like
it to be perfect (in format, and in issues such as this), I think we
have to live with it, acknowledging it but knowing the edge case does
not affect our usage.
Keeping the code in sync with the BCEL (not perfect) source is in our
interest to produce smaller changeset in any future updates. Would you
be okay with the current webrevs?
[1] https://issues.apache.org/jira/browse/BCEL-297
Best regards,
Joe
@Override
- public boolean equals(final Object o1, final Object o2) {
+ public boolean equals( final Object o1, final Object o2 ) {
final JavaClass THIS = (JavaClass) o1;
final JavaClass THAT = (JavaClass) o2;
- return THIS.getClassName().equals(THAT.getClassName());
+ return Objects.equals(THIS.getClassName(),
THAT.getClassName());
}
+
@Override
- public int hashCode(final Object o) {
+ public int hashCode( final Object o ) {
final JavaClass THIS = (JavaClass) o;
return THIS.getClassName().hashCode();
}
I've seen that idiom at a few places: equals was modified to
use Objects.equals(), but hashCode was not.
1. equals will throw if it is passed a null THIS or THAT (is
that intented?)
2. equals seems to think that getClassName() can return null,
whereas hashCode obviously expect that it will be non null.
I cannot say whether that's right or wrong, but I can smell something...
Maybe having a look at the issue that triggered the use of
Objects::equals could shed some light on this. It doesn't seem worse
than what was there before - it's just a bit odd.
best regards,
-- daniel
