Hi Xuelei, thanks for the comments!
#1: Removal of SecureSet from AbstractSet's hierarchy was a request made
by Sean earlier this month. One of his concerns was to insulate
SecureSet from changes/new methods added to AbstractSet, where the
superclass method wouldn't work well with the implementation of SecureSet.
#2: No problem. I'll look for any other instances of this and change them.
#3: I will change the run-time type check to look for SecureSet.
--Jamil
On 06/24/2014 08:51 AM, Xuelei Fan wrote:
1. Why make the following update? removeAll() is a method of AbstractSet.
private static class SecureSet<E>
- extends AbstractSet<E>
- implements java.io.Serializable {
+ implements Set<E>, java.io.Serializable {
2. Per Java coding conversions, please always use braces even for single
line conditional statement. For example:
1365 if (o == this)
1366 return true;
->
if (o == this) {
return true;
}
3. The implementation of SecureSet.equals() may not follow the spec of
equals() method. See item 8 of "Effective Java".
1368 if (!(o instanceof Set))
1369 return false;
->
1368 if (!(o instanceof SecureSet))
1369 return false;
Or I think you can re-use the spec of AbstractSet.equals() if SecureSet
extends AbstractSet.
Otherwise, looks fine to me.
Xuelei
On 6/20/2014 9:11 AM, Jamil Nimeh wrote:
Hello all,
This revision for the fix for 8015081 has the following test changes:
Removal of the static byte buffers for serialized data in lieu of a more
dynamic approach. A stripped down version of Subject in its own package
is now being compiled alongside SubjectNullTests.java. This version of
Subject allows the creation and serialization of Subjects with null
elements in the principals SecureSet. Immediately following
serialization, this special Subject's class designation is overwritten
to be javax.security.auth.Subject. When deserialization occurs, the
correct (JDK 9) version of Subject will be used and null element checks
will take place.
Thanks go to Weijun Wang for this testing approach.
http://cr.openjdk.java.net/~weijun/8015081/webrev.07
--Jamil
