Frederic, Looks good. And I like the refactoring.
A question - does the Descriptor field have the same potential for being null? e.g. in MBeanFeatureInfo.java new line 160 - do you need to check getDescriptor() == null as well? in MBeanConstructorinfo.java line 197, do you need the alternative null checking? MBeanAttributeInfo.java line 293? (there may be other lines, I didn't do a thorough search) thanks, Karen On Apr 13, 2012, at 9:01 AM, Staffan Larsen wrote: > This is much easier to read! > > In MBeanFeatureInfo.hasSameName() this check is not needed: > > 274 if(info.getName() == null) { > 275 return false; > 276 } > > since String.equals() with a null argument works as expected (return false). > The same is true for the implementation in hasSameDescription(). > > When calling hasSameName() and hasSameDescription() the "this." is not > needed, but perhaps using it makes the intent clearer? > > Thanks, > /Staffan > > > > > On 13 apr 2012, at 14:45, Frederic Parain wrote: > >> Thanks Staffan and Dmitry for your feedback, >> here's a new webrev with a refactored code for >> name and descriptions fields which are common to >> most MBean*Info classes: >> >> http://cr.openjdk.java.net/~fparain/7148497/webrev.01/ >> >> Thanks, >> >> Fred >> >> On 4/12/12 8:18 AM, Dmitry Samersoff wrote: >>> Frederic, >>> >>> I'm second to Staffan. >>> >>> Looks good to me but would prefer to have it refactored, e.g. create a >>> method equalOrNull(); >>> >>> -Dmitry >>> >>> >>> On 2012-04-10 18:33, Staffan Larsen wrote: >>>> Looks good! (Although I wish there was a better way to write code like >>>> this). >>>> >>>> /Staffan >>>> >>>> On 10 apr 2012, at 15:50, Frederic Parain wrote: >>>> >>>>> Greetings, >>>>> >>>>> This is a request for review for CR >>>>> 7148497: javax.management.MBeanAttributeInfo.hashCode throws >>>>> NullPointerException >>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7148497 >>>>> >>>>> Even if the initial CR only refers to MBeanAttributeInfo.hashCode(), >>>>> the problem is not limited to the MBeanAttributeInfo.hashCode(), but >>>>> is common to all javax.management.MBean*Info classes: >>>>> MBeanAttributeInfo, MBeanConstructorInfo, MBeanFeatureInfo, >>>>> MBeanInfo, MBeanNotificationInfo, MBeanOperationInfo and >>>>> MBeanParameterInfo. >>>>> >>>>> The root cause of the problem is that these classes can be >>>>> instantiated with some null fields (mainly, but not limited to, the >>>>> name and type fields), and these fields are dereferenced >>>>> unconditionally in some methods. Unconditional dereferencing is used >>>>> in the hashCode method but also in the equals() method. >>>>> >>>>> The proposed fix improves implementation of equals() and hashCode() >>>>> method to handle cases where some fields are null without throwing >>>>> a NPE. >>>>> >>>>> Webrev: >>>>> http://cr.openjdk.java.net/~fparain/7148497/webrev.00/ >>>>> >>>>> Thanks, >>>>> >>>>> Fred >>>>> >>>>> -- >>>>> Frederic Parain - Oracle >>>>> Grenoble Engineering Center - France >>>>> Phone: +33 4 76 18 81 17 >>>>> Email: frederic.par...@oracle.com >>>>> >>>> >>> >>> >> >> -- >> Frederic Parain - Oracle >> Grenoble Engineering Center - France >> Phone: +33 4 76 18 81 17 >> Email: frederic.par...@oracle.com >> >