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 <[email protected]>
Cc: Remi Forax <[email protected]>; core-libs-dev
<[email protected]>
Subject: Re: RFR: 8184693: (opt) add Optional.isEmpty
On 15 Apr 2018, at 11:25, Vivek Theeyarath <[email protected]> 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 <[email protected]>
Cc: core-libs-dev <[email protected]>
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:[email protected]]
Sent: Saturday, April 14, 2018 2:58 PM
To: Vivek Theeyarath <[email protected]>
Cc: core-libs-dev <[email protected]>
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" <[email protected]>
À: "core-libs-dev" <[email protected]>
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
