jmx-dev Codereview request for JDK-8023669: MBean*Info.hashCode : NPE

[Daniel Fuchs]

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?
>>>
>>>
>>