RFR: 8261625: Add `Elements.isAutomaticModule(ModuleElement)`

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

RFR: 8261625: Add `Elements.isAutomaticModule(ModuleElement)`

Joe Darcy-2
Addition of a small convenience method, with a default implementation, to Elements.

Most of the change is testing infrastructure. The negative cases, such as named modules, are tested in the style of other annotation processing tests for Elements utilities.

An automatic module comes from a jar file, so an API reading from a jar file is needed. To avoid piping in the awkward plumbing needed to do that directly in the annotation processing tests, I updated an existing automatic modules test which uses the tool box facility. My additions to the tool box might not be the done in the most idiomatic style for that API; happy to have guidance on how to proceed better there.

Assuming this general approach is agreed to, I'll update the imports on AutomaticModules.java to condense the text of the annotation processor source.

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

Commit messages:
 - Add test coverage.
 - Merge branch 'master' into 8261625
 - 8261625

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

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

Re: RFR: 8261625: Add `Elements.isAutomaticModule(ModuleElement)`

Joe Darcy-2
On Wed, 7 Apr 2021 18:49:38 GMT, Joe Darcy <[hidden email]> wrote:

> Addition of a small convenience method, with a default implementation, to Elements.
>
> Most of the change is testing infrastructure. The negative cases, such as named modules, are tested in the style of other annotation processing tests for Elements utilities.
>
> An automatic module comes from a jar file, so an API reading from a jar file is needed. To avoid piping in the awkward plumbing needed to do that directly in the annotation processing tests, I updated an existing automatic modules test which uses the tool box facility. My additions to the tool box might not be the done in the most idiomatic style for that API; happy to have guidance on how to proceed better there.
>
> Assuming this general approach is agreed to, I'll update the imports on AutomaticModules.java to condense the text of the annotation processor source.

PS And the CSR should be reviewed as well:

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

Thanks.

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

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

Re: RFR: 8261625: Add `Elements.isAutomaticModule(ModuleElement)`

Vicente Romero-3
In reply to this post by Joe Darcy-2
On Wed, 7 Apr 2021 18:49:38 GMT, Joe Darcy <[hidden email]> wrote:

> Addition of a small convenience method, with a default implementation, to Elements.
>
> Most of the change is testing infrastructure. The negative cases, such as named modules, are tested in the style of other annotation processing tests for Elements utilities.
>
> An automatic module comes from a jar file, so an API reading from a jar file is needed. To avoid piping in the awkward plumbing needed to do that directly in the annotation processing tests, I updated an existing automatic modules test which uses the tool box facility. My additions to the tool box might not be the done in the most idiomatic style for that API; happy to have guidance on how to proceed better there.
>
> Assuming this general approach is agreed to, I'll update the imports on AutomaticModules.java to condense the text of the annotation processor source.

test/langtools/tools/javac/modules/AutomaticModules.java line 694:

> 692:                      })
> 693:                 .writeAll()
> 694:                 .getOutputLines(Task.OutputKind.DIRECT);

I know this code predates your patch but now that you are here :) these two lines are unnecessary as the output is not kept. I mean you can remove:
           .writeAll()
           .getOutputLines(Task.OutputKind.DIRECT)

test/langtools/tools/javac/processing/model/util/elements/TestIsAutomaticMod.java line 2:

> 1: /*
> 2:  * Copyright (c) 2006, 2021, Oracle and/or its affiliates. All rights reserved.

2006 can be removed

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

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

Re: RFR: 8261625: Add `Elements.isAutomaticModule(ModuleElement)`

Vicente Romero-3
In reply to this post by Joe Darcy-2
On Wed, 7 Apr 2021 18:49:38 GMT, Joe Darcy <[hidden email]> wrote:

> Addition of a small convenience method, with a default implementation, to Elements.
>
> Most of the change is testing infrastructure. The negative cases, such as named modules, are tested in the style of other annotation processing tests for Elements utilities.
>
> An automatic module comes from a jar file, so an API reading from a jar file is needed. To avoid piping in the awkward plumbing needed to do that directly in the annotation processing tests, I updated an existing automatic modules test which uses the tool box facility. My additions to the tool box might not be the done in the most idiomatic style for that API; happy to have guidance on how to proceed better there.
>
> Assuming this general approach is agreed to, I'll update the imports on AutomaticModules.java to condense the text of the annotation processor source.

Changes requested by vromero (Reviewer).

test/langtools/tools/lib/toolbox/JavacTask.java line 341:

> 339:      * @return the result of calling {@code run}
> 340:      */
> 341:     public Result run(Expect expect, Processor... procs) {

I would prefer to add a new method to this class, something in the lines of:
public JavacTask processors(Processor... procs) {
    this.procs = List.of(procs);
    return this;
}

which I think is more in the spirit of the existing API, then you won't need to add the two new versions of the `run` method

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

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

Re: RFR: 8261625: Add `Elements.isAutomaticModule(ModuleElement)` [v2]

Joe Darcy-2
In reply to this post by Joe Darcy-2
> Addition of a small convenience method, with a default implementation, to Elements.
>
> Most of the change is testing infrastructure. The negative cases, such as named modules, are tested in the style of other annotation processing tests for Elements utilities.
>
> An automatic module comes from a jar file, so an API reading from a jar file is needed. To avoid piping in the awkward plumbing needed to do that directly in the annotation processing tests, I updated an existing automatic modules test which uses the tool box facility. My additions to the tool box might not be the done in the most idiomatic style for that API; happy to have guidance on how to proceed better there.
>
> Assuming this general approach is agreed to, I'll update the imports on AutomaticModules.java to condense the text of the annotation processor source.

Joe Darcy has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:

 - Respond to review feedback.
 - Merge branch 'master' into 8261625
 - Add test coverage.
 - Merge branch 'master' into 8261625
 - 8261625

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3382/files
  - new: https://git.openjdk.java.net/jdk/pull/3382/files/fdeadb26..5dab5c2f

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

  Stats: 2952 lines in 89 files changed: 2325 ins; 314 del; 313 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3382.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3382/head:pull/3382

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

Re: RFR: 8261625: Add `Elements.isAutomaticModule(ModuleElement)` [v2]

Joe Darcy-2
In reply to this post by Vicente Romero-3
On Thu, 8 Apr 2021 01:50:02 GMT, Vicente Romero <[hidden email]> wrote:

>> Joe Darcy has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
>>
>>  - Respond to review feedback.
>>  - Merge branch 'master' into 8261625
>>  - Add test coverage.
>>  - Merge branch 'master' into 8261625
>>  - 8261625
>
> test/langtools/tools/javac/processing/model/util/elements/TestIsAutomaticMod.java line 2:
>
>> 1: /*
>> 2:  * Copyright (c) 2006, 2021, Oracle and/or its affiliates. All rights reserved.
>
> 2006 can be removed

I copied a file from 2006 and then edited the result, hence I started with the 2006 year.

> test/langtools/tools/lib/toolbox/JavacTask.java line 341:
>
>> 339:      * @return the result of calling {@code run}
>> 340:      */
>> 341:     public Result run(Expect expect, Processor... procs) {
>
> I would prefer to add a new method to this class, something in the lines of:
> public JavacTask processors(Processor... procs) {
>     this.procs = List.of(procs);
>     return this;
> }
>
> which I think is more in the spirit of the existing API, then you won't need to add the two new versions of the `run` method

Added as suggested; thanks.

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

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

Re: RFR: 8261625: Add `Elements.isAutomaticModule(ModuleElement)` [v2]

Vicente Romero-3
In reply to this post by Joe Darcy-2
On Thu, 8 Apr 2021 06:25:37 GMT, Joe Darcy <[hidden email]> wrote:

>> Addition of a small convenience method, with a default implementation, to Elements.
>>
>> Most of the change is testing infrastructure. The negative cases, such as named modules, are tested in the style of other annotation processing tests for Elements utilities.
>>
>> An automatic module comes from a jar file, so an API reading from a jar file is needed. To avoid piping in the awkward plumbing needed to do that directly in the annotation processing tests, I updated an existing automatic modules test which uses the tool box facility. My additions to the tool box might not be the done in the most idiomatic style for that API; happy to have guidance on how to proceed better there.
>>
>> Assuming this general approach is agreed to, I'll update the imports on AutomaticModules.java to condense the text of the annotation processor source.
>
> Joe Darcy has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
>
>  - Respond to review feedback.
>  - Merge branch 'master' into 8261625
>  - Add test coverage.
>  - Merge branch 'master' into 8261625
>  - 8261625

lgtm

test/langtools/tools/javac/processing/model/util/elements/TestIsAutomaticMod.java line 53:

> 51:                 ModuleElement enclosing = elements.getModuleOf(e);
> 52:                 checkMod(enclosing, false);
> 53:                 System.out.println(enclosing.toString());

can this line `System.out...` be removed? probably a debugging leftover?

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

Marked as reviewed by vromero (Reviewer).

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

Re: RFR: 8261625: Add `Elements.isAutomaticModule(ModuleElement)` [v2]

Joe Darcy-2
On Thu, 8 Apr 2021 16:54:28 GMT, Vicente Romero <[hidden email]> wrote:

>> Joe Darcy has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
>>
>>  - Respond to review feedback.
>>  - Merge branch 'master' into 8261625
>>  - Add test coverage.
>>  - Merge branch 'master' into 8261625
>>  - 8261625
>
> test/langtools/tools/javac/processing/model/util/elements/TestIsAutomaticMod.java line 53:
>
>> 51:                 ModuleElement enclosing = elements.getModuleOf(e);
>> 52:                 checkMod(enclosing, false);
>> 53:                 System.out.println(enclosing.toString());
>
> can this line `System.out...` be removed? probably a debugging leftover?

Sure; I'll remove it before pushing.

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

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

Re: RFR: 8261625: Add `Elements.isAutomaticModule(ModuleElement)` [v3]

Joe Darcy-2
In reply to this post by Joe Darcy-2
> Addition of a small convenience method, with a default implementation, to Elements.
>
> Most of the change is testing infrastructure. The negative cases, such as named modules, are tested in the style of other annotation processing tests for Elements utilities.
>
> An automatic module comes from a jar file, so an API reading from a jar file is needed. To avoid piping in the awkward plumbing needed to do that directly in the annotation processing tests, I updated an existing automatic modules test which uses the tool box facility. My additions to the tool box might not be the done in the most idiomatic style for that API; happy to have guidance on how to proceed better there.
>
> Assuming this general approach is agreed to, I'll update the imports on AutomaticModules.java to condense the text of the annotation processor source.

Joe Darcy has updated the pull request incrementally with one additional commit since the last revision:

  Respond to review feedback.

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3382/files
  - new: https://git.openjdk.java.net/jdk/pull/3382/files/5dab5c2f..cea0aa16

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

  Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3382.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3382/head:pull/3382

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

Re: RFR: 8261625: Add `Elements.isAutomaticModule(ModuleElement)` [v3]

Jan Lahoda-3
On Thu, 8 Apr 2021 17:26:33 GMT, Joe Darcy <[hidden email]> wrote:

>> Addition of a small convenience method, with a default implementation, to Elements.
>>
>> Most of the change is testing infrastructure. The negative cases, such as named modules, are tested in the style of other annotation processing tests for Elements utilities.
>>
>> An automatic module comes from a jar file, so an API reading from a jar file is needed. To avoid piping in the awkward plumbing needed to do that directly in the annotation processing tests, I updated an existing automatic modules test which uses the tool box facility. My additions to the tool box might not be the done in the most idiomatic style for that API; happy to have guidance on how to proceed better there.
>>
>> Assuming this general approach is agreed to, I'll update the imports on AutomaticModules.java to condense the text of the annotation processor source.
>
> Joe Darcy has updated the pull request incrementally with one additional commit since the last revision:
>
>   Respond to review feedback.

LGTM

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

Marked as reviewed by jlahoda (Reviewer).

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

Integrated: 8261625: Add `Elements.isAutomaticModule(ModuleElement)`

Joe Darcy-2
In reply to this post by Joe Darcy-2
On Wed, 7 Apr 2021 18:49:38 GMT, Joe Darcy <[hidden email]> wrote:

> Addition of a small convenience method, with a default implementation, to Elements.
>
> Most of the change is testing infrastructure. The negative cases, such as named modules, are tested in the style of other annotation processing tests for Elements utilities.
>
> An automatic module comes from a jar file, so an API reading from a jar file is needed. To avoid piping in the awkward plumbing needed to do that directly in the annotation processing tests, I updated an existing automatic modules test which uses the tool box facility. My additions to the tool box might not be the done in the most idiomatic style for that API; happy to have guidance on how to proceed better there.
>
> Assuming this general approach is agreed to, I'll update the imports on AutomaticModules.java to condense the text of the annotation processor source.

This pull request has now been integrated.

Changeset: ccefa5e3
Author:    Joe Darcy <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/ccefa5e3
Stats:     194 lines in 5 files changed: 186 ins; 0 del; 8 mod

8261625: Add `Elements.isAutomaticModule(ModuleElement)`

Reviewed-by: vromero, jlahoda

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

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