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


Reply via email to