RFR 8072394: java.security.cert.PolicyQualifierInfo needs value-based equality

[Sean Mullan]

On 03/02/2015 09:24 AM, Florian Weimer wrote:
> 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.

What about adding it as an @implNote?

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

There is a typo on line 203: s/orded/ordered
Also, while you are in there, can you add an @Override to toString.

I can take care of filing an internal CCC and will let you know when 
that is approved or if there are any questions.

--Sean

> 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. :-)
>