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