On 9/11/13 2:05 AM, David Holmes wrote:
On 30/08/2013 12:11 AM, shanliang wrote:
Here is the new version:
http://cr.openjdk.java.net/~sjiang/jdk-8023669/01/
This seems good to address the NPE potential.
You are changing the values of the hashcodes though - is that a problem?
Hi David,
The algorithm by which hashCode() is computed isn't specified - so it
shouldn't
be an issue changing it - should it?
I must say I don't know how HashMap is serialized - for instance. So
assuming
we have an App1 sending a Map of MBeanInfos to an App2 over the wire, is
that
a problem if in App2 the MBeanInfo hashCodes resolve to different values?
I assume it doesn't...
I have never considered values of hashCode() to be part of the standard/API.
Since the algorithm isn't specified, then 2 implementations of the
standard should
be free to use different algorithms - and this shouldn't prevent then to
interoperate. So I hope we're safe in modifying the hashCode algorithm.
The important part is that it still satisfies the hashCode/equals contract.
cheers,
-- daniel
In javax/management/MBeanInfo.java
Objects.hash(getClassName(), getDescriptor().hashCode())
should, I think, be
Objects.hash(getClassName(), getDescriptor())
David
-----
Indeed, calling Objects.hash(Object ...) is a good idea, which
simplifies the code.
I used also Arrays.hashCode() to simplify the code, now the fix likes
really simple.
I have passed JCK tests, unit tests of management are passed too in my
desk.
toString() worked perfectly in the test, nothing to fix.
Shanliang
Daniel Fuchs wrote:
On 8/29/13 9:34 AM, shanliang wrote:
Hi,
Please review following fix, it addresses the issue only in the method
"hashCode":
bug: https://bugs.openjdk.java.net/browse/JDK-8023669
webrev: http://cr.openjdk.java.net/~sjiang/jdk-8023669/00/
Thanks,
Shanliang
Hi Shanliang,
<http://cr.openjdk.java.net/~sjiang/jdk-8023669/00/src/share/classes/javax/management/MBeanAttributeInfo.java.frames.html>
I suggest to simplify this by calling:
public int hashCode() {
return Objects.hash(getName(), getType());
}
(see
<http://docs.oracle.com/javase/7/docs/api/java/util/Objects.html#hash%28java.lang.Object...%29>)
<http://cr.openjdk.java.net/~sjiang/jdk-8023669/00/src/share/classes/javax/management/MBeanConstructorInfo.java.frames.html>
int hash = getName() == null ? 10 : getName().hashCode();
could be replaced by:
int hash = Objects.hashCode(getName());
Generally - and that stands for the other files you modified, you can
simplify things by replacing x.hashCode() with Objects.hashCode(x)
whenever there's the possibility that x can be null.
Note however that Objects is an API @since JDK 7 - so if you intend
to backport this fix to 6 & 5 you will need to alter your changeset
when backporting it.
MBeanOperationInfo.java, MBeanParameterInfo.java: I suggest
to use Objects.hash(...);
best regards,
-- daniel
BTW: one more question: you're also testing toString() in the test
and that's good - but are there any toString() that will require
fixing?
