Looks good.

--Sean

On 6/30/20 11:31 PM, Weijun Wang wrote:
Please review an updated fix at

    https://cr.openjdk.java.net/~weijun/8243592/webrev.03

After some discussion, we think there is no need to call contains(null) at all 
since it might not be reliable (see the new test). Now, collectionNullClean 
inspects all items in the input collection and create a new LinkedList.

Since there is no need to repopulate a list inside "new SecureSet", the new 
implementation is not slower than the original proposal from Tigran.

And there is no need for a CSR because there will be no behavior change.

Thanks,
Max

p.s. Welcome to the github playground at 
https://github.com/openjdk/playground/pull/9, but formal code review should 
stay in this mail thread.

On Apr 30, 2020, at 9:50 PM, Weijun Wang <weijun.w...@oracle.com> wrote:

Webrev updated at

   http://cr.openjdk.java.net/~weijun/8243592/webrev.02/

I removed my old test and rewrite the existing SubjectNullTests.java test in 
TestNG with quite some new test cases.

There are so many behavior changes (although we might say good programs are not 
affected) that I decided to add a CSR and a release note. Please also take a 
look.

            CSR : https://bugs.openjdk.java.net/browse/JDK-8244165
   release note : https://bugs.openjdk.java.net/browse/JDK-8244169

Thanks,
Max


On Apr 29, 2020, at 2:57 AM, Sean Mullan <sean.mul...@oracle.com> wrote:

I don't think containsAll and retainsAll need to call collectionNullClean 
either because SecureSet.contains(null) returns false now.

--Sean

On 4/28/20 9:58 AM, Weijun Wang wrote:
A new webrev is at
   http://cr.openjdk.java.net/~weijun/8243592/webrev.01/
I take this chance to do some formatting and add a new test. My first testng 
test!
Thanks,
Max
On Apr 28, 2020, at 8:16 PM, Sean Mullan <sean.mul...@oracle.com> wrote:

On 4/27/20 10:39 PM, Weijun Wang wrote:
Reading the Set spec, it looks like an NPE is still needed for add(), but 
remove() can be modified.

Good point, I agree.

--Sean

--Max
On Apr 27, 2020, at 10:27 PM, Sean Mullan <sean.mul...@oracle.com> wrote:

The fix looks fine to me. For consistency, you could make the same change for 
null elements in the other SecureSet methods: add, remove.

--Sean

On 4/25/20 3:39 AM, Weijun Wang wrote:
Please take a review at
   http://cr.openjdk.java.net/~weijun/8243592/webrev.00/
This is helpful if we do any set arithmetic between 2 Subject objects.
No new regression test, I intend to add a noreg-trivial label.
*Tigran*: Please confirm you are OK with the "Contributed-by" line.
Thanks,
Max


Reply via email to