Daniel Fuchs wrote:
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...
If app1 sends a Map<MBeanInfo, Object> to app2, the hashCode of each key
(MBeanInfo) will be re-calculated in app2, it is because the variable
"hashCode" of an MBeanOInfo is declared as "transient", the value
calculated in the app1 is not serialized and sent to the app2. So must
no problem here.
An exception is that we have Map<int, Object>, and the key "int" here is
hashCode of an MBeanInfo, if app1 sends this map to app2, then app2 uses
an MBenInfo's hashCode to look for a value in the map. But this seems
little possible.
Thanks,
Shanliang
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?
