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

[David Holmes]

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?

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