On 02/20/2015 11:23 PM, Sean Mullan wrote: > On 02/17/2015 09:30 AM, Florian Weimer wrote: >> On 02/16/2015 11:13 PM, Sean Mullan wrote: >> >>>> Based on that, PolicyQualifierInfo should have implemented value-based >>>> equals() and hashCode(), and the identity-based set is just a bug. >>>> (But >>>> the requirement I cited is a stronger requirement the Set would not >>>> enforce.) >>>> >>>> However, I think it's too late to fix this bug now. That's why I just >>>> added the identity counter. If you want the behavioral change instead, >>>> I can implement that as well. >>> >>> Maybe it's not too late. This is not a commonly used class, and the >>> compatibility risk is probably fairly low. If you code up the changes, I >>> can file a CCC on your behalf. >> >> Updated webrev: <http://cr.openjdk.java.net/~fweimer/8072394/webrev.01/> > > You need to add a description for the overridden equals/hashCode > methods, ex:
Okay, I didn't know that. I don't like specifying the hash code so strongly. But if we want to fix it at Arrays.hashCode(byte[]), it's probably better to implement Comparable as well, to aid the collision resolution in HashMap. The new webrev does that: <http://cr.openjdk.java.net/~fweimer/8072394/webrev.02/> I also compute the hashcode unconditionally in the constructor because PolicyQualifierInfo objects are used with sets, so their hashcode is so often needed. Funny how I wanted to make things go faster, and ended up introducing a slowdown. But correctness over performance. Most policy qualifier sets in X.509 certificates seem to have just one element, so I looked at optimizing that, but there's a lot of copying going on inside the implementation, and I'd have to special-case the one element case in multiple locations. Switching from a HashSet to a TreeSet would be externally visible, so that doesn't seem to be a good idea, either. We need an optimized immutable set class in the JDK. :-) -- Florian Weimer / Red Hat Product Security
