+1
There is one extra space in Optional.java: line 163: "is__not".
Thanks, Roger
On 4/18/18 2:06 AM, Stuart Marks wrote:
OK, looks good! +1 from me.
s'marks
On 4/17/18 10:34 PM, Vivek Theeyarath wrote:
Hi Stuart,
Done with the changes
http://cr.openjdk.java.net/~vtheeyarath/8184693/webrev.05/ .
Regards
Vivek
-----Original Message-----
From: Stuart Marks
Sent: Wednesday, April 18, 2018 8:56 AM
To: Vivek Theeyarath <vivek.theeyar...@oracle.com>
Cc: core-libs-dev <core-libs-dev@openjdk.java.net>; Paul Sandoz
<paul.san...@oracle.com>
Subject: Re: RFR: 8184693: (opt) add Optional.isEmpty
Hi Vivek,
Thanks for the update. In the test files, please remove the
unnecessary imports of List and the various Predicate types. In most
cases it's not a problem to have unnecessary imports. I happened to
notice in this case that they're left over from the previous version
of the webrev, and looking at the current webrev by itself, it's
clear they're unnecessary.
Thanks,
s'marks
On 4/17/18 6:23 AM, Vivek Theeyarath wrote:
Hi All,
Please find the updated webrev
http://cr.openjdk.java.net/~vtheeyarath/8184693/webrev.04
Regards
Vivek
-----Original Message-----
From: Stuart Marks
Sent: Tuesday, April 17, 2018 5:11 AM
To: Vivek Theeyarath <vivek.theeyar...@oracle.com>
Cc: core-libs-dev <core-libs-dev@openjdk.java.net>; Paul Sandoz
<paul.san...@oracle.com>
Subject: Re: RFR: 8184693: (opt) add Optional.isEmpty
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
