RFR: 8241187: ToolBox::grep should allow for negative filtering

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8241187: ToolBox::grep should allow for negative filtering

Guoxiong Li-2
Hi all,

This patch adds two methods in `ToolBox` to do the negative filtering. Although the label `noreg-self` was added, I write a test for this enhancement to verify the code. And the method name `grepNotMatch` may need to be improved. Any idea is appreciated.
 
Thank you for taking the time to review.

Best Regards.

-------------

Commit messages:
 - 8241187: ToolBox::grep should allow for negative filtering

Changes: https://git.openjdk.java.net/jdk/pull/1934/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1934&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8241187
  Stats: 80 lines in 2 files changed: 80 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1934.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1934/head:pull/1934

PR: https://git.openjdk.java.net/jdk/pull/1934
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8241187: ToolBox::grep should allow for negative filtering

Jonathan Gibbons-2
On Mon, 4 Jan 2021 17:17:58 GMT, Guoxiong Li <[hidden email]> wrote:

> Hi all,
>
> This patch adds two methods in `ToolBox` to do the negative filtering. Although the label `noreg-self` was added, I write a test for this enhancement to verify the code. And the method name `grepNotMatch` may need to be improved. Any idea is appreciated.
>  
> Thank you for taking the time to review.
>
> Best Regards.

test/langtools/tools/lib/toolbox/ToolBox.java line 204:

> 202:      * @return the strings not matching the regular expression
> 203:      */
> 204:     public List<String> grepNotMatch(Pattern pattern, List<String> lines) {

Instead of new methods named `grepNotMatch` I suggest adding new overloads of `grep` that take an additional `boolean invert` parameter that is conceptually equivalent to the `grep` `-v` option.  The existing `grep` methods can be updated to delegate to the new methods, passing `false` for the new parameter.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1934
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8241187: ToolBox::grep should allow for negative filtering [v2]

Guoxiong Li-2
In reply to this post by Guoxiong Li-2
> Hi all,
>
> This patch adds two methods in `ToolBox` to do the negative filtering. Although the label `noreg-self` was added, I write a test for this enhancement to verify the code. And the method name `grepNotMatch` may need to be improved. Any idea is appreciated.
>  
> Thank you for taking the time to review.
>
> Best Regards.

Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:

  Revise method and test.

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1934/files
  - new: https://git.openjdk.java.net/jdk/pull/1934/files/26b1589d..f55e8a3a

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1934&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1934&range=00-01

  Stats: 45 lines in 2 files changed: 27 ins; 2 del; 16 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1934.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1934/head:pull/1934

PR: https://git.openjdk.java.net/jdk/pull/1934
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8241187: ToolBox::grep should allow for negative filtering [v2]

Guoxiong Li-2
In reply to this post by Jonathan Gibbons-2
On Mon, 4 Jan 2021 18:11:14 GMT, Jonathan Gibbons <[hidden email]> wrote:

>> Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Revise method and test.
>
> test/langtools/tools/lib/toolbox/ToolBox.java line 204:
>
>> 202:      * @return the strings not matching the regular expression
>> 203:      */
>> 204:     public List<String> grepNotMatch(Pattern pattern, List<String> lines) {
>
> Instead of new methods named `grepNotMatch` I suggest adding new overloads of `grep` that take an additional `boolean invert` parameter that is conceptually equivalent to the `grep` `-v` option.  The existing `grep` methods can be updated to delegate to the new methods, passing `false` for the new parameter.

@jonathan-gibbons Thank you for your suggestion. The code is updated now.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1934
Reply | Threaded
Open this post in threaded view
|

Withdrawn: 8241187: ToolBox::grep should allow for negative filtering

duke
In reply to this post by Guoxiong Li-2
On Mon, 4 Jan 2021 17:17:58 GMT, Guoxiong Li <[hidden email]> wrote:

> Hi all,
>
> This patch adds two methods in `ToolBox` to do the negative filtering. Although the label `noreg-self` was added, I write a test for this enhancement to verify the code. And the method name `grepNotMatch` may need to be improved. Any idea is appreciated.
>  
> Thank you for taking the time to review.
>
> Best Regards.

This pull request has been closed without being integrated.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1934