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

[Jaroslav Bachorik]

Hi,

On 08/29/2013 04:43 PM, Daniel Fuchs wrote:
> On 8/29/13 4:11 PM, shanliang wrote:
>> Here is the new version:
>>     http://cr.openjdk.java.net/~sjiang/jdk-8023669/01/
>>
>> 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
> 
> Hi Shanliang,
> 
> This looks good! Thumbs up!

Looks good.

One minor nit is that
test/javax/management/MBeanInfo/MBeanInfoHashCodeNPETest.java:36 should
probably read "Tests that hashCode() does not throw NullPointerException"

-JB-

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