On 11/09/2013 5:37 PM, 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?
It shouldn't :) But that was why I asked. Even though the exact hashcode
is not part of the contract/spec we need to think about what impact it
might have just in case we suddenly realize it is a problem somewhere.
The main effect of the change would be on how maps store and traverse
these objects. Nobody should be relying on such details of course, but
there always seems to be someone who does.
Anyway I raised the question and it's been explored and I don't see any
flashing red lights so no problem. :)
Reviewed.
Cheers,
David
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?
