On 09/11/2013 09:54 AM, shanliang wrote: > 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.
Storing objects in a map by their hashcodes makes little sense. Two distinct objects can share the same hashcode easily - thus this kind of usage would mess up the application anyway. -JB- > > 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? >>>>> >>>>> >>>> >> >
