RE: RFR: 8164781: Pattern.asPredicate specification is incomplete

[Vivek Theeyarath]

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

[Paul Sandoz]

> On Apr 5, 2018, at 3: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 
> 

Nope, still not aligned to ~80 chars.


>> 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 
> 

Much better.

Paul.

Re: RFR: 8164781: Pattern.asPredicate specification is incomplete

[Roger Riggs]

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

[Vivek Theeyarath]

>> 
>> 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

[Paul Sandoz]

> 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
>> 

RE: RFR: 8164781: Pattern.asPredicate specification is incomplete

[Vivek Theeyarath]

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
> 

Re: RFR: 8164781: Pattern.asPredicate specification is incomplete

[Stuart Marks]

On 4/3/18 5:43 PM, Stuart Marks wrote:
Adding a method to create a Predicate that has match() semantics would be a fine 
task to consider separately.


I notice I had already filed a bug for this:

https://bugs.openjdk.java.net/browse/JDK-8184692

Feel free to pick it up.

s'marks

Re: RFR: 8164781: Pattern.asPredicate specification is incomplete

[Stuart Marks]

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  
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

Re: RFR: 8164781: Pattern.asPredicate specification is incomplete

[Martin Buchholz]

On Sun, Apr 1, 2018 at 3:56 AM, Uwe Schindler  wrote:

>
> In general, I'd prefer to have a predicate that uses matches() instead of
> find(). In my code I have never used find() because in most cases you want
> to match the whole string anyways. Of course you can use ^ and $ but I
> don't like that leniency. If I write code, I prefer to be explicit in the
> code what should done.
>

I have the opposite experience.  find()'s behavior is the default with grep
and perl regexes, so the behavior of matches() is the one I trip over.

Re: RFR: 8164781: Pattern.asPredicate specification is incomplete

[Paul Sandoz]

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  
> 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

Re: RFR: 8164781: Pattern.asPredicate specification is incomplete

[Uwe Schindler]

Hi,

In general, I'd prefer to have a predicate that uses matches() instead of 
find(). In my code I have never used find() because in most cases you want to 
match the whole string anyways. Of course you can use ^ and $ but I don't like 
that leniency. If I write code, I prefer to be explicit in the code what should 
done.

So I would prefer to add another asPredicate method that uses matches. I was 
stumbling on that several times.

Uwe

Am April 1, 2018 8:11:47 AM UTC schrieb Vivek Theeyarath 
:
>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