RFR: 8164781: Pattern.asPredicate specification is incomplete

[Paul Sandoz]

> On Apr 3, 2018, at 11:54 PM, Vivek Theeyarath <vivek.theeyarath at 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.theeyarath at oracle.com>
> Cc: Paul Sandoz <paul.sandoz at oracle.com>; Core-Libs-Dev <core-libs-dev at 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.theeyarath at 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
>>