RFR: 8177275: IllegalArgumentException when MH would have too many parameters is not specified for several methods
Hi All, Please review fix for https://bugs.openjdk.java.net/browse/JDK-8177275 . The jtreg test runs fine with the changes. http://cr.openjdk.java.net/~vtheeyarath/8177275/webrev.02/ CSR : https://bugs.openjdk.java.net/browse/JDK-8205917 Regards Vivek
RFR: 8204342: CLONE - methods in java.time's TCKZoneRules OpenJDK test miss @Test annotation
Hi All, Please review fix for https://bugs.openjdk.java.net/browse/JDK-8204342 . The jtreg test runs fine with the changes. http://cr.openjdk.java.net/~vtheeyarath/8204342/webrev.00/ Regards Vivek
RFR: 8202216: (bf) Add Buffer mismatch()
Hi All, Please review fix for https://bugs.openjdk.java.net/browse/JDK-8202216 Webrev: http://cr.openjdk.java.net/~vtheeyarath/8202216/webrev.00/ CSR : https://bugs.openjdk.java.net/browse/JDK-8204852 Regards Vivek
RE: RFR: 8193154: methods in java.time's TCKZoneRules OpenJDK test miss @Test annotation
Hi Lance, Thanks for the feedback. I did notice the @Test at the class level. The motivation behind the fix was a comment in the bug saying not having the annotation at the method level affects correct processing of TCKZoneRules and inclusion of test methods into master test list where this test has been ported to. So, I am assuming this would still help. But for that, I agree with you that this may not be needed. Regards Vivek From: Lance Andersen Sent: Saturday, May 26, 2018 6:06 PM To: Vivek Theeyarath <vivek.theeyar...@oracle.com> Cc: core-libs-dev <core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8193154: methods in java.time's TCKZoneRules OpenJDK test miss @Test annotation Hi Vivek, The original test has the @Test annotation on the class which indicates that all public methods are test methods: http://testng.org/doc/documentation-main.html#class-level So adding @Test to each method is OK but not needed. HTH Best Lance On May 26, 2018, at 3:51 AM, Vivek Theeyarath mailto:vivek.theeyar...@oracle.com"vivek.theeyar...@oracle.com> wrote: Hi All, Please review fix for https://bugs.openjdk.java.net/browse/JDK-8193154 . http://cr.openjdk.java.net/~vtheeyarath/8193154/webrev.00/ The test ran fine with Mach5 http://java.se.oracle.com:10065/mdash/jobs/root-jdk-dev-20180526-0645-24095 Regards Vivek http://oracle.com/us/design/oracle-email-sig-198324.gif HYPERLINK "http://oracle.com/us/design/oracle-email-sig-198324.gif; Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 HYPERLINK "mailto:lance.ander...@oracle.com"lance.ander...@oracle.com
RFR: 8193154: methods in java.time's TCKZoneRules OpenJDK test miss @Test annotation
Hi All, Please review fix for https://bugs.openjdk.java.net/browse/JDK-8193154 . http://cr.openjdk.java.net/~vtheeyarath/8193154/webrev.00/ The test ran fine with Mach5 http://java.se.oracle.com:10065/mdash/jobs/root-jdk-dev-20180526-0645-24095 Regards Vivek
RE: RFR: 8177276: MethodHandles.insertArguments doesn't specify IllegalArgumentException on index mismatch
Thanks for the comments Mandy. I have updated the test accordingly. http://cr.openjdk.java.net/~vtheeyarath/8177276/webrev.04/ Regards Vivek From: mandy chung Sent: Tuesday, May 22, 2018 10:07 PM To: Vivek Theeyarath <vivek.theeyar...@oracle.com>; Paul Sandoz <paul.san...@oracle.com> Cc: core-libs-dev <core-libs-dev@openjdk.java.net>; Nadeesh TV <nadeesh.thatathil.vala...@mdcpartners.be> Subject: Re: RFR: 8177276: MethodHandles.insertArguments doesn't specify IllegalArgumentException on index mismatch On 5/22/18 9:09 AM, Vivek Theeyarath wrote: Hi All, Thanks for the comments. I have incorporated the changes as per Nadeesh's and Paul's comments. The test runs fine with jtreg post the changes. Also, the typo errors Mandy pointed out has also been fixed. Please find the updated webrev. http://cr.openjdk.java.net/~vtheeyarath/8177276/webrev.03/ Looks fine. Nit: line 119-133 in MethodHandlesInsertArgumentsTest.java can be converted into two @Test(expected = ClassCastException.class) cases like the other two IAE cases. You can update it before you push. Mandy
RE: RFR: 8177276: MethodHandles.insertArguments doesn't specify IllegalArgumentException on index mismatch
Hi All,
Thanks for the comments. I have incorporated the changes as per
Nadeesh's and Paul's comments. The test runs fine with jtreg post the changes.
Also, the typo errors Mandy pointed out has also been fixed. Please find the
updated webrev.
http://cr.openjdk.java.net/~vtheeyarath/8177276/webrev.03/
Bug : https://bugs.openjdk.java.net/browse/JDK-8177276
CSR : https://bugs.openjdk.java.net/browse/JDK-8202991
Regards
Vivek
-Original Message-
From: Paul Sandoz
Sent: Monday, May 21, 2018 10:19 PM
To: Vivek Theeyarath <vivek.theeyar...@oracle.com>
Cc: core-libs-dev <core-libs-dev@openjdk.java.net>; Nadeesh TV
<nadeesh.thatathil.vala...@mdcpartners.be>
Subject: Re: RFR: 8177276: MethodHandles.insertArguments doesn't specify
IllegalArgumentException on index mismatch
> On May 19, 2018, at 7:14 AM, Nadeesh TV
> <nadeesh.thatathil.vala...@mdcpartners.be> wrote:
>
> Hi Vivek,
>
> IMHO, assigning back to methodHandle on line 109, 115, 122,123 is confusing
>
MethodHandlesInsertArgumentsTest.java
—
Yes, not just confusing but incorrect, the updated static field will affect
what is tested so you are dependent on the order of test method execution. Make
the field final and use the convention for static field names, and drop the
assignment for the insertArgument calls.
118 @Test
119 public void testInsertArgumentsPosZero() {
120 countTest();
121 try {
122 methodHandle = MethodHandles.insertArguments(methodHandle, 0,
"First");
123 methodHandle = MethodHandles.insertArguments(methodHandle, 1,
"First", new Object());
124 Assert.fail("ClassCastException not thrown");
125 }
126 catch(ClassCastException cce) {
127 }
128 }
You can split this out into multiple try/catch, one for each
MethodHandles.insertArguments call so the assertion directly captures the
failure point, otherwise declare the exception in the @Test.
MethodHandles.java
—
3489 * @throws ClassCastException If an argument does not mach the
corresponding bound parameter
Lower case the “If”
Paul.
FW: RFR: 8177276: MethodHandles.insertArguments doesn't specify IllegalArgumentException on index mismatch
Hi All, A gentle reminder seeking review. Regards Vivek From: Vivek Theeyarath Sent: Thursday, May 17, 2018 6:22 AM To: core-libs-dev@openjdk.java.net Subject: RFR: 8177276: MethodHandles.insertArguments doesn't specify IllegalArgumentException on index mismatch Hi All, Please review fix for https://bugs.openjdk.java.net/browse/JDK-8177276 . http://cr.openjdk.java.net/~vtheeyarath/8177276/webrev.02/ The related CSR is https://bugs.openjdk.java.net/browse/JDK-8202991 . Regards Vivek
RFR: 8177276: MethodHandles.insertArguments doesn't specify IllegalArgumentException on index mismatch
Hi All, Please review fix for https://bugs.openjdk.java.net/browse/JDK-8177276 . http://cr.openjdk.java.net/~vtheeyarath/8177276/webrev.02/ The related CSR is https://bugs.openjdk.java.net/browse/JDK-8202991 . Regards Vivek
RFR: 8195717: test java/lang/invoke/MethodHandlesTest timed out running testAsCollector1
Hi All, Please review fix for https://bugs.openjdk.java.net/browse/JDK-8195717 . http://cr.openjdk.java.net/~vtheeyarath/8195717/webrev.02/ The test class has been re factored into multiple classes with the existing MethodHandlesTest.java being made abstract. Testing Done: All the 7 test classes were run using jtreg with the additional VM params mentioned in the bug. Please find attached a text file with the relevant section of the jtr file (each test run 5 times) which includes the timing. Regards Vivek MethodHandlesCastFailureTest- #section:junit --messages:(4/370)-- command: junit -XX:+IgnoreUnrecognizedVMOptions -XX:-VerifyDependencies -esa test.java.lang.invoke.MethodHandlesCastFailureTest reason: User specified action: run junit/othervm/timeout=2500 -XX:+IgnoreUnrecognizedVMOptions -XX:-VerifyDependencies -esa test.java.lang.invoke.MethodHandlesCastFailureTest Mode: othervm [/othervm specified] elapsed time (seconds): 17.336 --configuration:(0/0)-- --System.out:(0/0)-- --System.err:(2/150)-- OpenJDK 64-Bit Server VM warning: Option AggressiveOpts was deprecated in version 11.0 and will likely be removed in a future release. STATUS:Passed. #section:junit --messages:(4/370)-- command: junit -XX:+IgnoreUnrecognizedVMOptions -XX:-VerifyDependencies -esa test.java.lang.invoke.MethodHandlesCastFailureTest reason: User specified action: run junit/othervm/timeout=2500 -XX:+IgnoreUnrecognizedVMOptions -XX:-VerifyDependencies -esa test.java.lang.invoke.MethodHandlesCastFailureTest Mode: othervm [/othervm specified] elapsed time (seconds): 17.199 --configuration:(0/0)-- --System.out:(0/0)-- --System.err:(2/150)-- OpenJDK 64-Bit Server VM warning: Option AggressiveOpts was deprecated in version 11.0 and will likely be removed in a future release. STATUS:Passed. #section:junit --messages:(4/370)-- command: junit -XX:+IgnoreUnrecognizedVMOptions -XX:-VerifyDependencies -esa test.java.lang.invoke.MethodHandlesCastFailureTest reason: User specified action: run junit/othervm/timeout=2500 -XX:+IgnoreUnrecognizedVMOptions -XX:-VerifyDependencies -esa test.java.lang.invoke.MethodHandlesCastFailureTest Mode: othervm [/othervm specified] elapsed time (seconds): 17.666 --configuration:(0/0)-- --System.out:(0/0)-- --System.err:(2/150)-- OpenJDK 64-Bit Server VM warning: Option AggressiveOpts was deprecated in version 11.0 and will likely be removed in a future release. STATUS:Passed. #section:junit --messages:(4/370)-- command: junit -XX:+IgnoreUnrecognizedVMOptions -XX:-VerifyDependencies -esa test.java.lang.invoke.MethodHandlesCastFailureTest reason: User specified action: run junit/othervm/timeout=2500 -XX:+IgnoreUnrecognizedVMOptions -XX:-VerifyDependencies -esa test.java.lang.invoke.MethodHandlesCastFailureTest Mode: othervm [/othervm specified] elapsed time (seconds): 17.636 --configuration:(0/0)-- --System.out:(0/0)-- --System.err:(2/150)-- OpenJDK 64-Bit Server VM warning: Option AggressiveOpts was deprecated in version 11.0 and will likely be removed in a future release. STATUS:Passed. #section:junit --messages:(4/370)-- command: junit -XX:+IgnoreUnrecognizedVMOptions -XX:-VerifyDependencies -esa test.java.lang.invoke.MethodHandlesCastFailureTest reason: User specified action: run junit/othervm/timeout=2500 -XX:+IgnoreUnrecognizedVMOptions -XX:-VerifyDependencies -esa test.java.lang.invoke.MethodHandlesCastFailureTest Mode: othervm [/othervm specified] elapsed time (seconds): 17.619 --configuration:(0/0)-- --System.out:(0/0)-- --System.err:(2/150)-- OpenJDK 64-Bit Server VM warning: Option AggressiveOpts was deprecated in version 11.0 and will likely be removed in a future release. STATUS:Passed. MethodHandlesAsCollectorTest- #section:junit --messages:(4/371)-- command: junit -XX:+IgnoreUnrecognizedVMOptions -XX:-VerifyDependencies -esa test.java.lang.invoke.MethodHandlesAsCollectorTest reason: User specified action: run junit/othervm/timeout=2500 -XX:+IgnoreUnrecognizedVMOptions -XX:-VerifyDependencies -esa test.java.lang.invoke.MethodHandlesAsCollectorTest Mode: othervm [/othervm specified] elapsed time (seconds): 896.002 --configuration:(0/0)-- --System.out:(0/0)-- --System.err:(2/150)-- OpenJDK 64-Bit Server VM warning: Option AggressiveOpts was deprecated in version 11.0 and will likely be removed in a future release. STATUS:Passed. #section:junit --messages:(4/371)-- command: junit -XX:+IgnoreUnrecognizedVMOptions -XX:-VerifyDependencies -esa
RE: RFR: 8184693: (opt) add Optional.isEmpty
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 >>
RE: RFR: 8184693: (opt) add Optional.isEmpty
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 >
RE: RFR: 8184693: (opt) add Optional.isEmpty
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
RE: RFR: 8184693: (opt) add Optional.isEmpty
Hi All, Please review http://cr.openjdk.java.net/~vtheeyarath/8184693/webrev.01/ 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
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
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
RE: RFR: 8184692: add Pattern.asMatchPredicate
Hi Roger, Please find the updated webrev http://cr.openjdk.java.net/~vtheeyarath/8184692/webrev.04/ . I fixed the indentation with my earlier fix (asPredicate) too. Hope that is fine. Regards Vivek From: Roger Riggs Sent: Thursday, April 12, 2018 7:43 PM To: Vivek Theeyarath <vivek.theeyar...@oracle.com> Cc: Core-Libs-Dev <core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8184692: add Pattern.asMatchPredicate Hi Vivek, ok, Generally, I like to see an updated webrev so that an old non-final webrev isn't left around to be a source of questions. Its easy enough to create a simple script to create the webrev and copy it to cr.openjdk... Continued javadoc @xxx lines should be indented to improve readability of the source. 5845 * against this pattern. Thanks, Roger On 4/12/18 8:49 AM, Vivek Theeyarath wrote: Hi, I have updated the doc as per the suggestion. Have finalized the csr too. Regards Vivek -Original Message- From: Paul Sandoz Sent: Thursday, April 12, 2018 12:40 AM To: Vivek Theeyarath HYPERLINK "mailto:vivek.theeyar...@oracle.com;<vivek.theeyar...@oracle.com> Cc: Roger Riggs HYPERLINK "mailto:roger.ri...@oracle.com;<roger.ri...@oracle.com>; Core-Libs-Dev HYPERLINK "mailto:core-libs-dev@openjdk.java.net;<core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8184692: add Pattern.asMatchPredicate HI, Hopefully this is the last update :-) To align with asPredicate and Roger’s prior guidance on that method i think you can just say this: Creates a predicate that tests if this pattern matches a given input string. which is similar to the language of Pattern.matches. No need for another webrev if this change is acceptable. Paul. On Apr 9, 2018, at 7:53 PM, Vivek Theeyarath HYPERLINK "mailto:vivek.theeyar...@oracle.com;<vivek.theeyar...@oracle.com> wrote: Thanks Paul / Roger for the inputs. I have updated the javadoc based on the inputs. http://cr.openjdk.java.net/~vtheeyarath/8184692/webrev.03 . Please review. Regards Vivek -Original Message- From: Paul Sandoz Sent: Monday, April 09, 2018 9:25 PM To: Roger Riggs HYPERLINK "mailto:roger.ri...@oracle.com;<roger.ri...@oracle.com> Cc: Core-Libs-Dev HYPERLINK "mailto:core-libs-dev@openjdk.java.net;<core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8184692: add Pattern.asMatchPredicate On Apr 9, 2018, at 8:41 AM, Roger Riggs HYPERLINK "mailto:roger.ri...@oracle.com;<roger.ri...@oracle.com> wrote: Hi Vivek, As with Pattern.asPredicate the first sentence can be improved. Creates a predicate that tests if this pattern matches the entire region. The region of what? region is clear from the context of a Matcher, but less so from the context of Pattern. Paul. 5833: As with the original issue, perhaps adding the word 'whole' or 'entire' will make it clearer that the pattern must match then entire input string. 5827: Split into two sentences, the second one starting "For example," 5840: add a blank line between methods Regards, Roger On 4/9/18 5:05 AM, Vivek Theeyarath wrote: Hi, Please find the updated webrev after incorporating Paul's comments. http://cr.openjdk.java.net/~vtheeyarath/8184692/webrev.02/ Also, I have created a csr for this issue https://bugs.openjdk.java.net/browse/JDK-8201308 . Regards Vivek -----Original Message- From: Paul Sandoz Sent: Friday, April 06, 2018 6:55 AM To: Vivek Theeyarath HYPERLINK "mailto:vivek.theeyar...@oracle.com;<vivek.theeyar...@oracle.com> Cc: Core-Libs-Dev HYPERLINK "mailto:core-libs-dev@openjdk.java.net;<core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8184692: add Pattern.asMatchPredicate On Apr 4, 2018, at 10:47 AM, Vivek Theeyarath HYPERLINK "mailto:vivek.theeyar...@oracle.com;<vivek.theeyar...@oracle.com> wrote: Hi All, Please review. Bug: https://bugs.openjdk.java.net/browse/JDK-8184692 Webrev : http://cr.openjdk.java.net/~vtheeyarath/8184692/webrev.00/ Like with your other patch, alignment to ~80 chars would be good, as that is mostly consistent with other code in the same source file. Let’s not use the word “find" here, so as not to confuse with matcher(s).find(). 5833 * @return The predicate which can be used for finding if an input string matches this pattern. I suggest: @return The predicate which can be used for matching an input string against this pattern You could also add a @see Matcher#matches Paul. The related jtreg test was run and the test passed . Regards Vivek
RE: RFR: 8184692: add Pattern.asMatchPredicate
Hi, I have updated the doc as per the suggestion. Have finalized the csr too. Regards Vivek -Original Message- From: Paul Sandoz Sent: Thursday, April 12, 2018 12:40 AM To: Vivek Theeyarath <vivek.theeyar...@oracle.com> Cc: Roger Riggs <roger.ri...@oracle.com>; Core-Libs-Dev <core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8184692: add Pattern.asMatchPredicate HI, Hopefully this is the last update :-) To align with asPredicate and Roger’s prior guidance on that method i think you can just say this: Creates a predicate that tests if this pattern matches a given input string. which is similar to the language of Pattern.matches. No need for another webrev if this change is acceptable. Paul. > On Apr 9, 2018, at 7:53 PM, Vivek Theeyarath <vivek.theeyar...@oracle.com> > wrote: > > Thanks Paul / Roger for the inputs. > I have updated the javadoc based on the inputs. > http://cr.openjdk.java.net/~vtheeyarath/8184692/webrev.03 . Please review. > > Regards > Vivek > -Original Message- > From: Paul Sandoz > Sent: Monday, April 09, 2018 9:25 PM > To: Roger Riggs <roger.ri...@oracle.com> > Cc: Core-Libs-Dev <core-libs-dev@openjdk.java.net> > Subject: Re: RFR: 8184692: add Pattern.asMatchPredicate > > > >> On Apr 9, 2018, at 8:41 AM, Roger Riggs <roger.ri...@oracle.com> wrote: >> >> Hi Vivek, >> >> As with Pattern.asPredicate the first sentence can be improved. >> >> Creates a predicate that tests if this pattern matches the entire region. >> > > The region of what? region is clear from the context of a Matcher, but less > so from the context of Pattern. > > Paul. > >> 5833: As with the original issue, perhaps adding the word 'whole' or >> 'entire' will make it clearer that the pattern must match then entire input >> string. >> > >> 5827: Split into two sentences, the second one starting "For example," >> >> 5840: add a blank line between methods >> >> Regards, Roger >> >> >> On 4/9/18 5:05 AM, Vivek Theeyarath wrote: >>> Hi, >>> Please find the updated webrev after incorporating Paul's comments. >>> http://cr.openjdk.java.net/~vtheeyarath/8184692/webrev.02/ >>> >>> Also, I have created a csr for this issue >>> https://bugs.openjdk.java.net/browse/JDK-8201308 . >>> >>> Regards >>> Vivek >>> -Original Message- >>> From: Paul Sandoz >>> Sent: Friday, April 06, 2018 6:55 AM >>> To: Vivek Theeyarath <vivek.theeyar...@oracle.com> >>> Cc: Core-Libs-Dev <core-libs-dev@openjdk.java.net> >>> Subject: Re: RFR: 8184692: add Pattern.asMatchPredicate >>> >>> >>> >>>> On Apr 4, 2018, at 10:47 AM, Vivek Theeyarath >>>> <vivek.theeyar...@oracle.com> wrote: >>>> >>>> Hi All, >>>> >>>> Please review. >>>> >>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8184692 >>>> >>>> Webrev : http://cr.openjdk.java.net/~vtheeyarath/8184692/webrev.00/ >>>> >>> Like with your other patch, alignment to ~80 chars would be good, as that >>> is mostly consistent with other code in the same source file. >>> >>> Let’s not use the word “find" here, so as not to confuse with >>> matcher(s).find(). >>> >>> 5833 * @return The predicate which can be used for finding if an >>> input string matches this pattern. >>> >>> I suggest: >>> >>> @return The predicate which can be used for matching an input >>> string against this pattern >>> >>> You could also add a @see Matcher#matches >>> >>> Paul. >>> >>>> >>>> The related jtreg test was run and the test passed . >>>> >>>> >>>> >>>> Regards >>>> >>>> Vivek >> >
RE: RFR: 8184692: add Pattern.asMatchPredicate
Thanks Paul / Roger for the inputs. I have updated the javadoc based on the inputs. http://cr.openjdk.java.net/~vtheeyarath/8184692/webrev.03 . Please review. Regards Vivek -Original Message- From: Paul Sandoz Sent: Monday, April 09, 2018 9:25 PM To: Roger Riggs <roger.ri...@oracle.com> Cc: Core-Libs-Dev <core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8184692: add Pattern.asMatchPredicate > On Apr 9, 2018, at 8:41 AM, Roger Riggs <roger.ri...@oracle.com> wrote: > > Hi Vivek, > > As with Pattern.asPredicate the first sentence can be improved. > > Creates a predicate that tests if this pattern matches the entire region. > The region of what? region is clear from the context of a Matcher, but less so from the context of Pattern. Paul. > 5833: As with the original issue, perhaps adding the word 'whole' or > 'entire' will make it clearer that the pattern must match then entire input > string. > > 5827: Split into two sentences, the second one starting "For example," > > 5840: add a blank line between methods > > Regards, Roger > > > On 4/9/18 5:05 AM, Vivek Theeyarath wrote: >> Hi, >> Please find the updated webrev after incorporating Paul's comments. >> http://cr.openjdk.java.net/~vtheeyarath/8184692/webrev.02/ >> >> Also, I have created a csr for this issue >> https://bugs.openjdk.java.net/browse/JDK-8201308 . >> >> Regards >> Vivek >> -Original Message- >> From: Paul Sandoz >> Sent: Friday, April 06, 2018 6:55 AM >> To: Vivek Theeyarath <vivek.theeyar...@oracle.com> >> Cc: Core-Libs-Dev <core-libs-dev@openjdk.java.net> >> Subject: Re: RFR: 8184692: add Pattern.asMatchPredicate >> >> >> >>> On Apr 4, 2018, at 10:47 AM, Vivek Theeyarath <vivek.theeyar...@oracle.com> >>> wrote: >>> >>> Hi All, >>> >>> Please review. >>> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8184692 >>> >>> Webrev : http://cr.openjdk.java.net/~vtheeyarath/8184692/webrev.00/ >>> >> Like with your other patch, alignment to ~80 chars would be good, as that is >> mostly consistent with other code in the same source file. >> >> Let’s not use the word “find" here, so as not to confuse with >> matcher(s).find(). >> >> 5833 * @return The predicate which can be used for finding if an input >> string matches this pattern. >> >> I suggest: >> >> @return The predicate which can be used for matching an input string >> against this pattern >> >> You could also add a @see Matcher#matches >> >> Paul. >> >>> >>> The related jtreg test was run and the test passed . >>> >>> >>> >>> Regards >>> >>> Vivek >
RE: RFR: 8184692: add Pattern.asMatchPredicate
Hi, Please find the updated webrev after incorporating Paul's comments. http://cr.openjdk.java.net/~vtheeyarath/8184692/webrev.02/ Also, I have created a csr for this issue https://bugs.openjdk.java.net/browse/JDK-8201308 . Regards Vivek -Original Message- From: Paul Sandoz Sent: Friday, April 06, 2018 6:55 AM To: Vivek Theeyarath <vivek.theeyar...@oracle.com> Cc: Core-Libs-Dev <core-libs-dev@openjdk.java.net> Subject: Re: RFR: 8184692: add Pattern.asMatchPredicate > On Apr 4, 2018, at 10:47 AM, Vivek Theeyarath <vivek.theeyar...@oracle.com> > wrote: > > Hi All, > > Please review. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8184692 > > Webrev : http://cr.openjdk.java.net/~vtheeyarath/8184692/webrev.00/ > Like with your other patch, alignment to ~80 chars would be good, as that is mostly consistent with other code in the same source file. Let’s not use the word “find" here, so as not to confuse with matcher(s).find(). 5833 * @return The predicate which can be used for finding if an input string matches this pattern. I suggest: @return The predicate which can be used for matching an input string against this pattern You could also add a @see Matcher#matches Paul. > > > The related jtreg test was run and the test passed . > > > > Regards > > Vivek
RE: RFR: 8164781: Pattern.asPredicate specification is incomplete
Hi Roger, The sentence has been changed and here is the updated one http://cr.openjdk.java.net/~rraghavan/8164781/webrev.04/ . The line length has also been fixed as suggested by Paul to adhere to ~80 chars. I have finalized the csr too. Regards Vivek -Original Message- From: Roger Riggs Sent: Thursday, April 05, 2018 6:35 PM To: core-libs-dev@openjdk.java.net Subject: Re: RFR: 8164781: Pattern.asPredicate specification is incomplete Hi Vivek, Can we do something to make the first sentence less confusing? How about: * Creates a predicate that tests a given input string for a subsequence that matches this pattern. -or- * Creates a predicate that tests if this pattern is found in a given input string. Thanks, Roger On 4/5/18 6:34 AM, Vivek Theeyarath wrote: >>> Hi, >>> I have incorporated the changes as per the feedback and here is the >>> updated webrev . >>> http://cr.openjdk.java.net/~rraghavan/8164781/webrev.02/ . >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8164781 >>> >> +1 > Thanks Paul > >> I know it’s picky, but would you mind sticking closer to the existing >> line length in the source file (no need for another review) > Here is my attempt. Hope it is better now. > http://cr.openjdk.java.net/~rraghavan/8164781/webrev.03 > >> Did you run the jtreg test to verify it passes? I missed the problem >> initially, glad Stuart caught it, but i presume the test would of reported a >> failure? if not there is something wrong with the test itself that should be >> investigated. > > I missed it earlier. As you rightly pointed out, without Stuart's inputs the > test fails and it passes with it as expected. > >>> Here is the related csr >>> https://bugs.openjdk.java.net/browse/JDK-8200603 >>> >> Ok, i tweaked some of the information (after creating a CSR one often needs >> to edit it to fill in the gaps). >> Can you blockquote the markdown for the embedded patch since the formatting >> is all messed up? > I have restored the formatting. Hope it would suffice. > https://bugs.openjdk.java.net/browse/JDK-8200603 > > Regards > Vivek
RE: RFR: 8164781: Pattern.asPredicate specification is incomplete
>> >> Hi, >> I have incorporated the changes as per the feedback and here is the >> updated webrev . >> http://cr.openjdk.java.net/~rraghavan/8164781/webrev.02/ . >>Bug: https://bugs.openjdk.java.net/browse/JDK-8164781 >> >+1 Thanks Paul >I know it’s picky, but would you mind sticking closer to the existing line >length in the source file >(no need for another review) Here is my attempt. Hope it is better now. http://cr.openjdk.java.net/~rraghavan/8164781/webrev.03 >Did you run the jtreg test to verify it passes? I missed the problem >initially, glad Stuart caught it, but i presume the test would of reported a >failure? if not there is something wrong with the test itself that should be >investigated. I missed it earlier. As you rightly pointed out, without Stuart's inputs the test fails and it passes with it as expected. >> Here is the related csr https://bugs.openjdk.java.net/browse/JDK-8200603 >> >Ok, i tweaked some of the information (after creating a CSR one often needs to >edit it to fill in the gaps). >Can you blockquote the markdown for the embedded patch since the formatting is >all messed up? I have restored the formatting. Hope it would suffice. https://bugs.openjdk.java.net/browse/JDK-8200603 Regards Vivek
RFR: 8184692: add Pattern.asMatchPredicate
Hi All, Please review. Bug: https://bugs.openjdk.java.net/browse/JDK-8184692 Webrev : http://cr.openjdk.java.net/~vtheeyarath/8184692/webrev.00/ The related jtreg test was run and the test passed . Regards Vivek
RE: RFR: 8164781: Pattern.asPredicate specification is incomplete
Hi,
I have incorporated the changes as per the feedback and here is the
updated webrev .
http://cr.openjdk.java.net/~rraghavan/8164781/webrev.02/ .
Bug: https://bugs.openjdk.java.net/browse/JDK-8164781
Here is the related csr https://bugs.openjdk.java.net/browse/JDK-8200603
I will try to address Uwe's point with a fix separately.
Regards
Vivek
-Original Message-
From: Stuart Marks
Sent: Wednesday, April 04, 2018 6:13 AM
To: Vivek Theeyarath <vivek.theeyar...@oracle.com>
Cc: Paul Sandoz <paul.san...@oracle.com>; Core-Libs-Dev
<core-libs-dev@openjdk.java.net>
Subject: Re: RFR: 8164781: Pattern.asPredicate specification is incomplete
Hi Vivek,
Thanks for taking on this task.
In case it wasn't clear from Paul's mail, what I think you should do is
continue with this fix as a doc-only (and test-only) change, and not modify the
behavior of this method. Doing that would be an incompatible change.
Uwe's point is a reasonable one, which is that you can't tell from the method
name "asPredicate" whether it uses find() or match() semantics. Oh well, I
think we just have to live with this, and document it clearly.
Adding a method to create a Predicate that has match() semantics would be a
fine task to consider separately.
Also, in RegExTest.java,
4686 if (p.test("word1234")) {
4687 failCount++;
4688 }
I think the logic should be negated, as the predicate should properly find the
pattern in this string.
Thanks,
s'marks
On 4/2/18 10:56 AM, Paul Sandoz wrote:
> Hi,
>
> Looks good, expect for:
>
> 5823 * @return The predicate which can be used for finding on a string
>
> “finding on a… ” is a little awkward to parse . I recommend to either change
> it back, since the first sentence of the method doc says what it means by
> matches, or being a little more verbose:
>
>The predicate which can be used for finding a match on a
> subsequence of a string
>
> You will need a CSR to document the clarification in specification behavior.
>
> —
>
> To Uwe’s point, we could have chosen a more descriptive method name, e.g.
> asFinding/Predicate, leaving logical space for say any future
> asMatching/Predicate if we chose to add it.
>
> Paul.
>
>
>> On Apr 1, 2018, at 1:11 AM, Vivek Theeyarath <vivek.theeyar...@oracle.com>
>> wrote:
>>
>> Hi all,
>>
>>Please review.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8164781
>>
>> Webrev: http://cr.openjdk.java.net/~rraghavan/8164781/webrev.01/
>>
>>
>>
>> Regards
>>
>> Vivek
>
RFR: 8164781: Pattern.asPredicate specification is incomplete
Hi all, Please review. Bug: https://bugs.openjdk.java.net/browse/JDK-8164781 Webrev: http://cr.openjdk.java.net/~rraghavan/8164781/webrev.01/ Regards Vivek
