Hi Vivek,

Please add "@since 11" tags to the doc comments of the four Optional*.isEmpty() methods.

Regarding the tests, I don't think the various newly added testIsEmpty() tests are useful. The setup in those test files already generates a fairly comprehensive set of Optional values from a variety of sources. All the assertion checking is performed in the checkEmpty() and checkPresent() methods. It should be sufficient to add an assertTrue(isEmpty()) call to checkEmpty() -- as you've done -- and also to add an assertFalse(isEmpty()) call to checkPresent(). If these assertions are added, the additional testIsEmpty() test is unnecessary.

This applies to all four of the regression test files.

Thanks,

s'marks

On 4/16/18 11:08 AM, Vivek Theeyarath wrote:
Hi All,
        Please find the updated webrev 
http://cr.openjdk.java.net/~vtheeyarath/8184693/webrev.02/ .
Here is the csr which I have raised for this change 
https://bugs.openjdk.java.net/browse/JDK-8201606

Regards
Vivek
-----Original Message-----
From: Chris Hegarty
Sent: Sunday, April 15, 2018 6:48 PM
To: Vivek Theeyarath <vivek.theeyar...@oracle.com>
Cc: Remi Forax <fo...@univ-mlv.fr>; core-libs-dev 
<core-libs-dev@openjdk.java.net>
Subject: Re: RFR: 8184693: (opt) add Optional.isEmpty



On 15 Apr 2018, at 11:25, Vivek Theeyarath <vivek.theeyar...@oracle.com> wrote:

Hi All,
    Please review http://cr.openjdk.java.net/~vtheeyarath/8184693/webrev.01/

This looks ok to me.

For consistency, can you please update the copyright header year range in 
OptionalInt.

-Chris.

Regards
Vivek
-----Original Message-----
From: Vivek Theeyarath
Sent: Saturday, April 14, 2018 6:24 PM
To: Remi Forax <fo...@univ-mlv.fr>
Cc: core-libs-dev <core-libs-dev@openjdk.java.net>
Subject: RE: RFR: 8184693: (opt) add Optional.isEmpty

I missed that Remi. Thanks for pointing it out. Will address those and get back.

Regards
Vivek
-----Original Message-----
From: Remi Forax [mailto:fo...@univ-mlv.fr]
Sent: Saturday, April 14, 2018 2:58 PM
To: Vivek Theeyarath <vivek.theeyar...@oracle.com>
Cc: core-libs-dev <core-libs-dev@openjdk.java.net>
Subject: Re: RFR: 8184693: (opt) add Optional.isEmpty

Hi Vivek,
OptionalInt, OptionalLong and OptionalDouble should be changed too.

Rémi

----- Mail original -----
De: "Vivek Theeyarath" <vivek.theeyar...@oracle.com>
À: "core-libs-dev" <core-libs-dev@openjdk.java.net>
Envoyé: Samedi 14 Avril 2018 08:22:50
Objet: RFR: 8184693: (opt) add Optional.isEmpty

Hi All,

              Please review.

Bug: https://bugs.openjdk.java.net/browse/JDK-8184693

Webrev : http://cr.openjdk.java.net/~vtheeyarath/8184693/webrev.00/



The related jtreg test was run and the test passed .



Regards

Vivek

Reply via email to