> On Apr 3, 2018, at 11:54 PM, Vivek Theeyarath <vivek.theeyar...@oracle.com> > 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 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) 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. > 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? Thanks, Paul. > 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 >>
