Re: RFR 8054536: sun.security.x509.Extension object may throw NPE for hashCode and equals method

2016-07-25 Thread Wang Weijun
Hi Svetlana > http://cr.openjdk.java.net/~snikandrova/8054536/webrev.03/ > Change looks fine. One nit: we usually combine the null check and the assignment into a single line. For example, this.x = Objects.requireNonNull(x). > >

Re: RFR 8054536: sun.security.x509.Extension object may throw NPE for hashCode and equals method

2016-07-25 Thread Svetlana Nikandrova
Hi Max, yes, comment states that this constructor is only for sub-classes so I've made it protected. Compilation didn't broke (at least in jdk and tests). AFAIK extensionId should never be null, as for extensionValue there are extensions without value (e.g. OCSPNocheck) but in that case value

Re: RFR 8054536: sun.security.x509.Extension object may throw NPE for hashCode and equals method

2016-07-18 Thread Wang Weijun
If the constructor is only meant to be called by a child class, can we just change it to protected? Also, if extensionId and extensionValue should never be null, we should focus on not to create an Extension that has one of 2 fields as null. Inside this class, we can add Objects::requireNonNull

Re: RFR 8054536: sun.security.x509.Extension object may throw NPE for hashCode and equals method

2016-07-18 Thread Svetlana Nikandrova
Artem, please see updated review: http://cr.openjdk.java.net/~snikandrova/8054536/webrev.02/ Thanks, Svetlana On 15.07.2016 21:32, Artem Smotrakov wrote: Hi Svetlana, The webrev looks fine to me. Please see one more comment inli

Re: RFR 8054536: sun.security.x509.Extension object may throw NPE for hashCode and equals method

2016-07-15 Thread Artem Smotrakov
Hi Svetlana, The webrev looks fine to me. Please see one more comment inline. On 07/15/2016 11:12 AM, Svetlana Nikandrova wrote: Hi Artem, ok, lets get rid of possible NPE in other methods too: http://cr.openjdk.java.net/~snikandrova/8054536/webrev.01/

Re: RFR 8054536: sun.security.x509.Extension object may throw NPE for hashCode and equals method

2016-07-15 Thread Svetlana Nikandrova
Hi Artem, ok, lets get rid of possible NPE in other methods too: http://cr.openjdk.java.net/~snikandrova/8054536/webrev.01/ As for unnecessary try-catch in test I'd prefer to have it to emphasis that we are checking for NPE. I'm n

Re: RFR 8054536: sun.security.x509.Extension object may throw NPE for hashCode and equals method

2016-07-14 Thread Artem Smotrakov
Hi Svetlana, I'll leave the main review to an official reviewer, but I have a couple of comments. There are a couple of other places in Extension.java where NPE may occur: - line 255: I see that "extensionId" is checked for null in other methods, but not in getId() - line 200: I see that "ext

RFR 8054536: sun.security.x509.Extension object may throw NPE for hashCode and equals method

2016-07-14 Thread Svetlana Nikandrova
Hello, please review the fix for: https://bugs.openjdk.java.net/browse/JDK-8054536 Webrev: http://cr.openjdk.java.net/~snikandrova/8054536/webrev.00/ Description: Equals and hasCode methods of Extension use extensionId without prio