Codereview request for JDK-8023669: MBean*Info.hashCode : NPE
shanliang
shanliang.jiang at oracle.com
Tue Sep 10 23:44:12 PDT 2013
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?
Good question!
It should not be a problem. The value "hashCode" is declared as "private
transient", so it must be always same for an MBeanInfo instance within a VM.
>
> In javax/management/MBeanInfo.java
>
> Objects.hash(getClassName(), getDescriptor().hashCode())
>
> should, I think, be
>
> Objects.hash(getClassName(), getDescriptor())
Yes you are right, corrected in the new version:
http://cr.openjdk.java.net/~sjiang/jdk-8023669/02/
thanks,
Shanliang
>
> 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?
>>>
>>>
>>
More information about the serviceability-dev
mailing list