Re: RFR: 8332522: SwitchBootstraps::mappedEnumLookup constructs unused array [v5]
> For general pattern matching switches, the `SwitchBootstraps` class currently
> generates a cascade of `if`-like statements, computing the correct target
> case index for the given input.
>
> There is one special case which permits a relatively easy faster handling,
> and that is when all the case labels case enum constants (but the switch is
> still a "pattern matching" switch, as tranditional enum switches do not go
> through `SwitchBootstraps`). Like:
>
>
> enum E {A, B, C}
> E e = ...;
> switch (e) {
> case null -> {}
> case A a -> {}
> case C c -> {}
> case B b -> {}
> }
>
>
> We can create an array mapping the runtime ordinal to the appropriate case
> number, which is somewhat similar to what javac is doing for ordinary
> switches over enums.
>
> The `SwitchBootstraps` class was trying to do so, when the restart index is
> zero, but failed to do so properly, so that code is not used (and does not
> actually work properly).
>
> This patch is trying to fix that - when all the case labels are enum
> constants, an array mapping the runtime enum ordinals to the case number will
> be created (lazily), for restart index == 0. And this map will then be used
> to quickly produce results for the given input. E.g. for the case above, the
> mapping will be `{0 -> 0, 1 -> 2, 2 -> 1}` (meaning `{A -> 0, B -> 2, C ->
> 1}`).
>
> When the restart index is != 0 (i.e. when there's a guard in the switch, and
> the guard returned `false`), the if cascade will be generated lazily and
> used, as in the general case. If it would turn out there are significant
> enum-only switches with guards/restart index != 0, we could improve there as
> well, by generating separate mappings for every (used) restart index.
>
> I believe the current tests cover the code functionally sufficiently - see
> `SwitchBootstrapsTest.testEnums`. It is only that the tests do not (and
> regression tests cannot easily, I think) differentiate whether the
> special-case or generic implementation is used.
>
> I've added a new microbenchmark attempting to demonstrate the difference.
> There are two benchmarks, both having only enum constants as case labels.
> One, `enumSwitchTraditional` is an "old" switch, desugared fully by javac,
> the other, `enumSwitchWithBootstrap` is an equivalent switch that uses the
> `SwitchBootstraps`. Before this patch, I was getting values like:
>
> Benchmark Mode Cnt Score Error Units
> SwitchEnum.enumSwitchTraditionalavgt 15 11.719 ± 0.333 ns/op
> SwitchEnum.enumSwitchWithBootstrap avgt 15 24.668 ± 1.037 ...
Jan Lahoda has updated the pull request incrementally with one additional
commit since the last revision:
Reflecting review feedback - using instanceof rather than isAssignableFrom
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19906/files
- new: https://git.openjdk.org/jdk/pull/19906/files/512a802a..d09fa40e
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19906=04
- incr: https://webrevs.openjdk.org/?repo=jdk=19906=03-04
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/19906.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19906/head:pull/19906
PR: https://git.openjdk.org/jdk/pull/19906
Re: RFR: 8332522: SwitchBootstraps::mappedEnumLookup constructs unused array [v4]
On Thu, 27 Jun 2024 19:30:39 GMT, Jan Lahoda wrote:
>> For general pattern matching switches, the `SwitchBootstraps` class
>> currently generates a cascade of `if`-like statements, computing the correct
>> target case index for the given input.
>>
>> There is one special case which permits a relatively easy faster handling,
>> and that is when all the case labels case enum constants (but the switch is
>> still a "pattern matching" switch, as tranditional enum switches do not go
>> through `SwitchBootstraps`). Like:
>>
>>
>> enum E {A, B, C}
>> E e = ...;
>> switch (e) {
>> case null -> {}
>> case A a -> {}
>> case C c -> {}
>> case B b -> {}
>> }
>>
>>
>> We can create an array mapping the runtime ordinal to the appropriate case
>> number, which is somewhat similar to what javac is doing for ordinary
>> switches over enums.
>>
>> The `SwitchBootstraps` class was trying to do so, when the restart index is
>> zero, but failed to do so properly, so that code is not used (and does not
>> actually work properly).
>>
>> This patch is trying to fix that - when all the case labels are enum
>> constants, an array mapping the runtime enum ordinals to the case number
>> will be created (lazily), for restart index == 0. And this map will then be
>> used to quickly produce results for the given input. E.g. for the case
>> above, the mapping will be `{0 -> 0, 1 -> 2, 2 -> 1}` (meaning `{A -> 0, B
>> -> 2, C -> 1}`).
>>
>> When the restart index is != 0 (i.e. when there's a guard in the switch, and
>> the guard returned `false`), the if cascade will be generated lazily and
>> used, as in the general case. If it would turn out there are significant
>> enum-only switches with guards/restart index != 0, we could improve there as
>> well, by generating separate mappings for every (used) restart index.
>>
>> I believe the current tests cover the code functionally sufficiently - see
>> `SwitchBootstrapsTest.testEnums`. It is only that the tests do not (and
>> regression tests cannot easily, I think) differentiate whether the
>> special-case or generic implementation is used.
>>
>> I've added a new microbenchmark attempting to demonstrate the difference.
>> There are two benchmarks, both having only enum constants as case labels.
>> One, `enumSwitchTraditional` is an "old" switch, desugared fully by javac,
>> the other, `enumSwitchWithBootstrap` is an equivalent switch that uses the
>> `SwitchBootstraps`. Before this patch, I was getting values like:
>>
>> Benchmark Mode Cnt Score Error Units
>> SwitchEnum.enumSwitchTraditionalavgt 15 11.719 ± 0.333 ns/op
>> Swi...
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Clone the labels.
Any input on the current version of the patch?
-
PR Comment: https://git.openjdk.org/jdk/pull/19906#issuecomment-2203073610
Re: RFR: 8335274: SwitchBootstraps.ResolvedEnumLabels.resolvedEnum should be final
On Fri, 28 Jun 2024 09:33:41 GMT, Aleksey Shipilev wrote: > > FWIW, there are more `@Stable` uses here: #19906 > > if you would have a moment to check that, it may be helpful. Thanks. > > That one looks fine: it is set outside of constructor, is intrinsically racy, > and it has AFAICS the recovery paths when we read `null` out of > `MappedEnumCache.generatedSwitch` or `MappedEnumCache.constantsMap`. Thanks Aleksey! - PR Comment: https://git.openjdk.org/jdk/pull/19933#issuecomment-2197287772
Re: RFR: 8335274: SwitchBootstraps.ResolvedEnumLabels.resolvedEnum should be final
On Thu, 27 Jun 2024 19:30:34 GMT, Aleksey Shipilev wrote: > I was auditing the current uses of `@Stable` before relaxing its barriers > ([JDK-8333791](https://bugs.openjdk.org/browse/JDK-8333791)), and this is an > easy spot. > > `resolvedEnum` is not `final`. So technically publishing the object via data > race can show `resolvedEnum` as `null`, which would break `test()` that does > not expect it. Currently not a practical problem since its safety is covered > by adjacent `final` fields, but should be more precise. FWIW, there are more `@Stable` uses here: https://github.com/openjdk/jdk/pull/19906 if you would have a moment to check that, it may be helpful. Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/19933#issuecomment-2196499286
Re: RFR: 8335274: SwitchBootstraps.ResolvedEnumLabels.resolvedEnum should be final
On Thu, 27 Jun 2024 19:30:34 GMT, Aleksey Shipilev wrote: > I was auditing the current uses of `@Stable` before relaxing its barriers > ([JDK-8333791](https://bugs.openjdk.org/browse/JDK-8333791)), and this is an > easy spot. > > `resolvedEnum` is not `final`. So technically publishing the object via data > race can show `resolvedEnum` as `null`, which would break `test()` that does > not expect it. Currently not a practical problem since its safety is covered > by adjacent `final` fields, but should be more precise. Looks good to me. Marked as reviewed by jlahoda (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19933#pullrequestreview-2147390630 PR Review: https://git.openjdk.org/jdk/pull/19933#pullrequestreview-2147390890
Re: RFR: 8332522: SwitchBootstraps::mappedEnumLookup constructs unused array [v3]
On Thu, 27 Jun 2024 19:07:07 GMT, ExE Boss wrote: > Since `labels` is no longer eagerly cloned, it’s important to store the > converted labels into a local array to avoid leaking them to user code. True. But it is easier and cleaner, IMO, to simply put back cloning of the labels. - PR Comment: https://git.openjdk.org/jdk/pull/19906#issuecomment-2195515947
Re: RFR: 8332522: SwitchBootstraps::mappedEnumLookup constructs unused array [v4]
> For general pattern matching switches, the `SwitchBootstraps` class currently
> generates a cascade of `if`-like statements, computing the correct target
> case index for the given input.
>
> There is one special case which permits a relatively easy faster handling,
> and that is when all the case labels case enum constants (but the switch is
> still a "pattern matching" switch, as tranditional enum switches do not go
> through `SwitchBootstraps`). Like:
>
>
> enum E {A, B, C}
> E e = ...;
> switch (e) {
> case null -> {}
> case A a -> {}
> case C c -> {}
> case B b -> {}
> }
>
>
> We can create an array mapping the runtime ordinal to the appropriate case
> number, which is somewhat similar to what javac is doing for ordinary
> switches over enums.
>
> The `SwitchBootstraps` class was trying to do so, when the restart index is
> zero, but failed to do so properly, so that code is not used (and does not
> actually work properly).
>
> This patch is trying to fix that - when all the case labels are enum
> constants, an array mapping the runtime enum ordinals to the case number will
> be created (lazily), for restart index == 0. And this map will then be used
> to quickly produce results for the given input. E.g. for the case above, the
> mapping will be `{0 -> 0, 1 -> 2, 2 -> 1}` (meaning `{A -> 0, B -> 2, C ->
> 1}`).
>
> When the restart index is != 0 (i.e. when there's a guard in the switch, and
> the guard returned `false`), the if cascade will be generated lazily and
> used, as in the general case. If it would turn out there are significant
> enum-only switches with guards/restart index != 0, we could improve there as
> well, by generating separate mappings for every (used) restart index.
>
> I believe the current tests cover the code functionally sufficiently - see
> `SwitchBootstrapsTest.testEnums`. It is only that the tests do not (and
> regression tests cannot easily, I think) differentiate whether the
> special-case or generic implementation is used.
>
> I've added a new microbenchmark attempting to demonstrate the difference.
> There are two benchmarks, both having only enum constants as case labels.
> One, `enumSwitchTraditional` is an "old" switch, desugared fully by javac,
> the other, `enumSwitchWithBootstrap` is an equivalent switch that uses the
> `SwitchBootstraps`. Before this patch, I was getting values like:
>
> Benchmark Mode Cnt Score Error Units
> SwitchEnum.enumSwitchTraditionalavgt 15 11.719 ± 0.333 ns/op
> SwitchEnum.enumSwitchWithBootstrap avgt 15 24.668 ± 1.037 ...
Jan Lahoda has updated the pull request incrementally with one additional
commit since the last revision:
Clone the labels.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19906/files
- new: https://git.openjdk.org/jdk/pull/19906/files/d7c97845..512a802a
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19906=03
- incr: https://webrevs.openjdk.org/?repo=jdk=19906=02-03
Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/19906.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19906/head:pull/19906
PR: https://git.openjdk.org/jdk/pull/19906
Re: RFR: 8332522: SwitchBootstraps::mappedEnumLookup constructs unused array [v3]
> For general pattern matching switches, the `SwitchBootstraps` class currently
> generates a cascade of `if`-like statements, computing the correct target
> case index for the given input.
>
> There is one special case which permits a relatively easy faster handling,
> and that is when all the case labels case enum constants (but the switch is
> still a "pattern matching" switch, as tranditional enum switches do not go
> through `SwitchBootstraps`). Like:
>
>
> enum E {A, B, C}
> E e = ...;
> switch (e) {
> case null -> {}
> case A a -> {}
> case C c -> {}
> case B b -> {}
> }
>
>
> We can create an array mapping the runtime ordinal to the appropriate case
> number, which is somewhat similar to what javac is doing for ordinary
> switches over enums.
>
> The `SwitchBootstraps` class was trying to do so, when the restart index is
> zero, but failed to do so properly, so that code is not used (and does not
> actually work properly).
>
> This patch is trying to fix that - when all the case labels are enum
> constants, an array mapping the runtime enum ordinals to the case number will
> be created (lazily), for restart index == 0. And this map will then be used
> to quickly produce results for the given input. E.g. for the case above, the
> mapping will be `{0 -> 0, 1 -> 2, 2 -> 1}` (meaning `{A -> 0, B -> 2, C ->
> 1}`).
>
> When the restart index is != 0 (i.e. when there's a guard in the switch, and
> the guard returned `false`), the if cascade will be generated lazily and
> used, as in the general case. If it would turn out there are significant
> enum-only switches with guards/restart index != 0, we could improve there as
> well, by generating separate mappings for every (used) restart index.
>
> I believe the current tests cover the code functionally sufficiently - see
> `SwitchBootstrapsTest.testEnums`. It is only that the tests do not (and
> regression tests cannot easily, I think) differentiate whether the
> special-case or generic implementation is used.
>
> I've added a new microbenchmark attempting to demonstrate the difference.
> There are two benchmarks, both having only enum constants as case labels.
> One, `enumSwitchTraditional` is an "old" switch, desugared fully by javac,
> the other, `enumSwitchWithBootstrap` is an equivalent switch that uses the
> `SwitchBootstraps`. Before this patch, I was getting values like:
>
> Benchmark Mode Cnt Score Error Units
> SwitchEnum.enumSwitchTraditionalavgt 15 11.719 ± 0.333 ns/op
> SwitchEnum.enumSwitchWithBootstrap avgt 15 24.668 ± 1.037 ...
Jan Lahoda has updated the pull request incrementally with one additional
commit since the last revision:
Update src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java
Co-authored-by: Chen Liang
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19906/files
- new: https://git.openjdk.org/jdk/pull/19906/files/79b3a76f..d7c97845
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19906=02
- incr: https://webrevs.openjdk.org/?repo=jdk=19906=01-02
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/19906.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19906/head:pull/19906
PR: https://git.openjdk.org/jdk/pull/19906
Re: RFR: 8332522: SwitchBootstraps::mappedEnumLookup constructs unused array [v2]
> For general pattern matching switches, the `SwitchBootstraps` class currently
> generates a cascade of `if`-like statements, computing the correct target
> case index for the given input.
>
> There is one special case which permits a relatively easy faster handling,
> and that is when all the case labels case enum constants (but the switch is
> still a "pattern matching" switch, as tranditional enum switches do not go
> through `SwitchBootstraps`). Like:
>
>
> enum E {A, B, C}
> E e = ...;
> switch (e) {
> case null -> {}
> case A a -> {}
> case C c -> {}
> case B b -> {}
> }
>
>
> We can create an array mapping the runtime ordinal to the appropriate case
> number, which is somewhat similar to what javac is doing for ordinary
> switches over enums.
>
> The `SwitchBootstraps` class was trying to do so, when the restart index is
> zero, but failed to do so properly, so that code is not used (and does not
> actually work properly).
>
> This patch is trying to fix that - when all the case labels are enum
> constants, an array mapping the runtime enum ordinals to the case number will
> be created (lazily), for restart index == 0. And this map will then be used
> to quickly produce results for the given input. E.g. for the case above, the
> mapping will be `{0 -> 0, 1 -> 2, 2 -> 1}` (meaning `{A -> 0, B -> 2, C ->
> 1}`).
>
> When the restart index is != 0 (i.e. when there's a guard in the switch, and
> the guard returned `false`), the if cascade will be generated lazily and
> used, as in the general case. If it would turn out there are significant
> enum-only switches with guards/restart index != 0, we could improve there as
> well, by generating separate mappings for every (used) restart index.
>
> I believe the current tests cover the code functionally sufficiently - see
> `SwitchBootstrapsTest.testEnums`. It is only that the tests do not (and
> regression tests cannot easily, I think) differentiate whether the
> special-case or generic implementation is used.
>
> I've added a new microbenchmark attempting to demonstrate the difference.
> There are two benchmarks, both having only enum constants as case labels.
> One, `enumSwitchTraditional` is an "old" switch, desugared fully by javac,
> the other, `enumSwitchWithBootstrap` is an equivalent switch that uses the
> `SwitchBootstraps`. Before this patch, I was getting values like:
>
> Benchmark Mode Cnt Score Error Units
> SwitchEnum.enumSwitchTraditionalavgt 15 11.719 ± 0.333 ns/op
> SwitchEnum.enumSwitchWithBootstrap avgt 15 24.668 ± 1.037 ...
Jan Lahoda has updated the pull request incrementally with one additional
commit since the last revision:
Reflecting review feedback.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19906/files
- new: https://git.openjdk.org/jdk/pull/19906/files/8ab9ee64..79b3a76f
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19906=01
- incr: https://webrevs.openjdk.org/?repo=jdk=19906=00-01
Stats: 17 lines in 1 file changed: 9 ins; 3 del; 5 mod
Patch: https://git.openjdk.org/jdk/pull/19906.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19906/head:pull/19906
PR: https://git.openjdk.org/jdk/pull/19906
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v9]
On Mon, 24 Jun 2024 12:57:39 GMT, Jorn Vernee wrote: >> This PR adds a new JDK tool, called `jnativescan`, that can be used to find >> code that accesses native functionality. Currently this includes `native` >> method declarations, and methods marked with `@Restricted`. >> >> The tool accepts a list of class path and module path entries through >> `--class-path` and `--module-path`, and a set of root modules through >> `--add-modules`, as well as an optional target release with `--release`. >> >> The default mode is for the tool to report all uses of `@Restricted` >> methods, and `native` method declaration in a tree-like structure: >> >> >> app.jar (ALL-UNNAMED): >> main.Main: >> main.Main::main(String[])void references restricted methods: >> java.lang.foreign.MemorySegment::reinterpret(long)MemorySegment >> main.Main::m()void is a native method declaration >> >> >> The `--print-native-access` option can be used print out all the module >> names of modules doing native access in a comma separated list. For class >> path code, this will print out `ALL-UNNAMED`. >> >> Testing: >> - `langtools_jnativescan` tests. >> - Running the tool over jextract's libclang bindings, which use the FFM API, >> and thus has a lot of references to `@Restricted` methods. >> - tier 1-3 > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > de-dupe on path, not module name Looks good to me. - Marked as reviewed by jlahoda (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19774#pullrequestreview-2145411864
Re: RFR: 8332522: SwitchBootstraps::mappedEnumLookup constructs unused array
On Wed, 26 Jun 2024 12:46:18 GMT, Claes Redestad wrote:
>> For general pattern matching switches, the `SwitchBootstraps` class
>> currently generates a cascade of `if`-like statements, computing the correct
>> target case index for the given input.
>>
>> There is one special case which permits a relatively easy faster handling,
>> and that is when all the case labels case enum constants (but the switch is
>> still a "pattern matching" switch, as tranditional enum switches do not go
>> through `SwitchBootstraps`). Like:
>>
>>
>> enum E {A, B, C}
>> E e = ...;
>> switch (e) {
>> case null -> {}
>> case A a -> {}
>> case C c -> {}
>> case B b -> {}
>> }
>>
>>
>> We can create an array mapping the runtime ordinal to the appropriate case
>> number, which is somewhat similar to what javac is doing for ordinary
>> switches over enums.
>>
>> The `SwitchBootstraps` class was trying to do so, when the restart index is
>> zero, but failed to do so properly, so that code is not used (and does not
>> actually work properly).
>>
>> This patch is trying to fix that - when all the case labels are enum
>> constants, an array mapping the runtime enum ordinals to the case number
>> will be created (lazily), for restart index == 0. And this map will then be
>> used to quickly produce results for the given input. E.g. for the case
>> above, the mapping will be `{0 -> 0, 1 -> 2, 2 -> 1}` (meaning `{A -> 0, B
>> -> 2, C -> 1}`).
>>
>> When the restart index is != 0 (i.e. when there's a guard in the switch, and
>> the guard returned `false`), the if cascade will be generated lazily and
>> used, as in the general case. If it would turn out there are significant
>> enum-only switches with guards/restart index != 0, we could improve there as
>> well, by generating separate mappings for every (used) restart index.
>>
>> I believe the current tests cover the code functionally sufficiently - see
>> `SwitchBootstrapsTest.testEnums`. It is only that the tests do not (and
>> regression tests cannot easily, I think) differentiate whether the
>> special-case or generic implementation is used.
>>
>> I've added a new microbenchmark attempting to demonstrate the difference.
>> There are two benchmarks, both having only enum constants as case labels.
>> One, `enumSwitchTraditional` is an "old" switch, desugared fully by javac,
>> the other, `enumSwitchWithBootstrap` is an equivalent switch that uses the
>> `SwitchBootstraps`. Before this patch, I was getting values like:
>>
>> Benchmark Mode Cnt Score Error Units
>> SwitchEnum.enumSwitchTraditionalavgt 15 11.719 ± 0.333 ns/op
>> Swi...
>
> test/micro/org/openjdk/bench/java/lang/runtime/SwitchEnum.java line 57:
>
>> 55: for (E e : inputs) {
>> 56: sum += switch (e) {
>> 57: case null -> -1;
>
> As this `null` case adds a case relative to the `-Traditional` test then
> maybe removing one of the `E0, E1, ...` cases would make the test a little
> bit more apples-to-apples?
Using `case null -> ` will push javac to use the new code, but all switches do
some kind of null check for the selector value. The difference is that if
there's no `case null`, there will be `Objects.requireNonNull` generated for
the selector value (which will throw an NPE if the value is `null`), while here
it will jump to the given case.
So, `case null` does not have the same weight as a normal case, so I don't
think it would be fair to remove a full case to compensate for it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19906#discussion_r1654784712
RFR: 8332522: SwitchBootstraps::mappedEnumLookup constructs unused array
For general pattern matching switches, the `SwitchBootstraps` class currently
generates a cascade of `if`-like statements, computing the correct target case
index for the given input.
There is one special case which permits a relatively easy faster handling, and
that is when all the case labels case enum constants (but the switch is still a
"pattern matching" switch, as tranditional enum switches do not go through
`SwitchBootstraps`). Like:
enum E {A, B, C}
E e = ...;
switch (e) {
case null -> {}
case A a -> {}
case C c -> {}
case B b -> {}
}
We can create an array mapping the runtime ordinal to the appropriate case
number, which is somewhat similar to what javac is doing for ordinary switches
over enums.
The `SwitchBootstraps` class was trying to do so, when the restart index is
zero, but failed to do so properly, so that code is not used (and does not
actually work properly).
This patch is trying to fix that - when all the case labels are enum constants,
an array mapping the runtime enum ordinals to the case number will be created
(lazily), for restart index == 0. And this map will then be used to quickly
produce results for the given input. E.g. for the case above, the mapping will
be `{0 -> 0, 1 -> 2, 2 -> 1}` (meaning `{A -> 0, B -> 2, C -> 1}`).
When the restart index is != 0 (i.e. when there's a guard in the switch, and
the guard returned `false`), the if cascade will be generated lazily and used,
as in the general case. If it would turn out there are significant enum-only
switches with guards/restart index != 0, we could improve there as well, by
generating separate mappings for every (used) restart index.
I believe the current tests cover the code functionally sufficiently - see
`SwitchBootstrapsTest.testEnums`. It is only that the tests do not (and
regression tests cannot easily, I think) differentiate whether the special-case
or generic implementation is used.
I've added a new microbenchmark attempting to demonstrate the difference. There
are two benchmarks, both having only enum constants as case labels. One,
`enumSwitchTraditional` is an "old" switch, desugared fully by javac, the
other, `enumSwitchWithBootstrap` is an equivalent switch that uses the
`SwitchBootstraps`. Before this patch, I was getting values like:
Benchmark Mode Cnt Score Error Units
SwitchEnum.enumSwitchTraditionalavgt 15 11.719 ± 0.333 ns/op
SwitchEnum.enumSwitchWithBootstrap avgt 15 24.668 ± 1.037 ns/op
and with this patch:
Benchmark Mode Cnt Score Error Units
SwitchEnum.enumSwitchTraditionalavgt 15 11.550 ± 0.157 ns/op
SwitchEnum.enumSwitchWithBootstrap avgt 15 13.225 ± 0.173 ns/op
So, this seems like a clear improvement to me.
-
Commit messages:
- 8332522: SwitchBootstraps::mappedEnumLookup constructs unused array
Changes: https://git.openjdk.org/jdk/pull/19906/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=19906=00
Issue: https://bugs.openjdk.org/browse/JDK-8332522
Stats: 187 lines in 2 files changed: 149 ins; 18 del; 20 mod
Patch: https://git.openjdk.org/jdk/pull/19906.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19906/head:pull/19906
PR: https://git.openjdk.org/jdk/pull/19906
Re: RFR: 8333308: javap --system handling doesn't work on internal class names
On Tue, 25 Jun 2024 13:59:35 GMT, Sonia Zaldana Calles wrote: > Hi all, > > This PR addresses [JDK-808](https://bugs.openjdk.org/browse/JDK-808) > enabling `javap -system` to handle internal class names. > > Thanks, > Sonia For a test, I agree it is quite difficult. For the `--system` parameter, it would probably be possible to create a directory (e.g. `$DIR`), and inside it `$DIR/lib`, and copy suitable `jrt-fs.jar` and `modules` to it from `$JDK/lib`. And the use `--system $DIR`. But, it would still be fairly difficult to be sure the class' content is read from `$DIR`, not from the runtime JDK - `modules` would presumably need to be modified/different than the one for the runtime JDK, and that is not very easy. - PR Comment: https://git.openjdk.org/jdk/pull/19883#issuecomment-2191232755
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v7]
On Fri, 21 Jun 2024 18:37:01 GMT, Jorn Vernee wrote: >> src/jdk.jdeps/share/classes/com/sun/tools/jdeprscan/Main.java line 417: >> >>> 415: return false; >>> 416: } >>> 417: JavaFileManager fm = >>> pp.getPlatformTrusted(release).getFileManager(); >> >> Not sure if this change is necessary. I believe `release` is verified to be >> a valid platform name at this point, so even with the new check, it should >> still work. (And `getPlatformTrusted` could possibly be eliminated.) But >> maybe I am missing something? > > I wanted to avoid adding exception handling code here, since > `PlatformNotSupported` is a checked exception. Ah, OK, I didn't realize that. Seems fine then! - PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1649315863
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v5]
On Thu, 20 Jun 2024 17:44:54 GMT, Alan Bateman wrote: >> src/jdk.jdeps/share/classes/com/sun/tools/jnativescan/JNativeScanTask.java >> line 113: >> >>> 111: // Class-Path attribute specifies that jars that >>> 112: // are not found are simply ignored. Do the same >>> here >>> 113: classPathJars.offer(otherJar); >> >> The expansion of the class path looks right but the Class-Path attribute is >> awkward to deal with as its relative URI rather than a file path. More on >> this this later. > > Not important for now but the expansion will probably need to"visited" set to > detect cycles (yes, it is possible). > > In addition to cycles, it is possible to have several JAR files with the same > Class-Path value, e.g. lib/logging.jar, so a visited set would be useful for > that too. FWIW, javac has `com.sun.tools.javac.file.FSInfo.getJarClassPath`, which is not transitive, and is implemented a bit differently, but I wonder if there's a chance for some code sharing. Maybe not now, but eventually. - PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1649296334
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v7]
On Thu, 20 Jun 2024 19:45:29 GMT, Jorn Vernee wrote:
>> This PR adds a new JDK tool, called `jnativescan`, that can be used to find
>> code that accesses native functionality. Currently this includes `native`
>> method declarations, and methods marked with `@Restricted`.
>>
>> The tool accepts a list of class path and module path entries through
>> `--class-path` and `--module-path`, and a set of root modules through
>> `--add-modules`, as well as an optional target release with `--release`.
>>
>> The default mode is for the tool to report all uses of `@Restricted`
>> methods, and `native` method declaration in a tree-like structure:
>>
>>
>> app.jar (ALL-UNNAMED):
>> main.Main:
>> main.Main::main(String[])void references restricted methods:
>> java.lang.foreign.MemorySegment::reinterpret(long)MemorySegment
>> main.Main::m()void is a native method declaration
>>
>>
>> The `--print-native-access` option can be used print out all the module
>> names of modules doing native access in a comma separated list. For class
>> path code, this will print out `ALL-UNNAMED`.
>>
>> Testing:
>> - `langtools_jnativescan` tests.
>> - Running the tool over jextract's libclang bindings, which use the FFM API,
>> and thus has a lot of references to `@Restricted` methods.
>> - tier 1-3
>
> Jorn Vernee has updated the pull request incrementally with one additional
> commit since the last revision:
>
> add extra test for missing root modules
Overall looks great to me. Some minor comments inline.
src/jdk.compiler/share/classes/com/sun/tools/javac/platform/JDKPlatformProvider.java
line 87:
> 85: @Override
> 86: public PlatformDescription getPlatform(String platformName, String
> options) throws PlatformNotSupported {
> 87: if (Source.lookup(platformName) == null) {
`Source.lookup` is probably not the right way to check whether the platform is
supported - the `Source` enum may (and does) contain versions for which we
don't have the historical API record. I think
`SUPPORTED_JAVA_PLATFORM_VERSIONS.contains(platformName)` would work better,
since the content is read directly from `ct.sym`.
src/jdk.jdeps/share/classes/com/sun/tools/jdeprscan/Main.java line 417:
> 415: return false;
> 416: }
> 417: JavaFileManager fm =
> pp.getPlatformTrusted(release).getFileManager();
Not sure if this change is necessary. I believe `release` is verified to be a
valid platform name at this point, so even with the new check, it should still
work. (And `getPlatformTrusted` could possibly be eliminated.) But maybe I am
missing something?
src/jdk.jdeps/share/classes/com/sun/tools/jnativescan/ClassResolver.java line
125:
> 123: private static Map
> packageToSystemModule(JavaFileManager platformFileManager) {
> 124: try {
> 125: Set locations =
> platformFileManager.listLocationsForModules(
FWIW: +1 on using the modules from the `ct.sym` rather than runtime modules
here!
test/langtools/tools/jnativescan/TestJNativeScan.java line 174:
> 172: "-add-modules",
> "org.singlejar,org.myapp",
> 173: "--print-native-access"))
> 174: .stdoutShouldContain("org.singlejar")
It is a small thing, bu was there a consideration for a stronger assert on the
output, checking that the output is precisely something like
`--enable-native-access org.lib,org.service,org.singlejar`? Would require that
the output is stable, which may be tricky, but also not a bad property. Just an
idea for consideration.
-
PR Review: https://git.openjdk.org/jdk/pull/19774#pullrequestreview-2133198642
PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1649303668
PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1649307310
PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1649297021
PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1649294137
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v7]
On Thu, 20 Jun 2024 16:58:55 GMT, Alan Bateman wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/platform/JDKPlatformProvider.java
>> line 93:
>>
>>> 91: }
>>> 92:
>>> 93: public PlatformDescription getPlatformTrusted(String platformName) {
>>
>> I noticed that `getPlatform` was not throwing an exception if the release
>> was not valid, which then later results in an NPE. I've added an explicit
>> check here instead. The caller can then catch the exception.
>
> I see the "options" arg is not used so maybe another approach here is a
> one-arg getPlatform that throws PlatformNotSupported and then migrate the
> other usages at another time.
>
> (This is just a passing comment, not important for the core of this feature).
We probably can eliminate the `options` (which is a historical artifact), but I
don't think it needs to be done here
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1649307870
Integrated: 8333086: Using Console.println is unnecessarily slow due to JLine initalization
On Thu, 30 May 2024 13:50:33 GMT, Jan Lahoda wrote:
> Consider these two programs:
>
>
> public class SystemPrint {
> public static void main(String... args) {
> System.err.println("Hello!");
> }
> }
>
> and:
>
> public class IOPrint {
> public static void main(String... args) {
> java.io.IO.println("Hello!");
> }
> }
>
>
> They do the same conceptual thing - write a text to the output. But,
> `IO.println` delegates to `Console.println`, which then delegates to a
> `Console` backend, and the default backend is currently based on JLine.
>
> The issues is that JLine takes a quite a long time to initialize, and in a
> program like this, JLine is not really needed - it is used to provide better
> editing experience when reading input, but there's no reading in these
> programs.
>
> For example, on my computer:
>
> $ time java -classpath /tmp SystemPrint
> Hello!
>
> real0m0,035s
> user0m0,019s
> sys 0m0,019s
>
> $ time java -classpath /tmp --enable-preview IOPrint
> Hello!
>
> real0m0,165s
> user0m0,324s
> sys 0m0,042s
>
>
> The proposal herein is to delegate to the simpler `Console` backend from
> `java.base` as long as the user only uses methods that print to output, and
> switch to the JLine delegate when other methods (typically input) is used.
> Note that while technically `writer()` is a method doing output, it will
> force JLine initialization to avoid possible problems if the client caches
> the writer and uses it after switching the delegates.
>
> With this patch, I can get timing like this:
>
> $ time java --enable-preview -classpath /tmp/ IOPrint
> Hello!
>
> real0m0,051s
> user0m0,038s
> sys 0m0,020s
>
>
> which seems much more acceptable.
>
> There is also #19467, which may reduce the time further.
>
> A future work might focus on making JLine initialize faster, but avoiding
> JLine initialization in case where we don't need it seems like a good step to
> me in any case.
This pull request has now been integrated.
Changeset: f7dbb98f
Author:Jan Lahoda
URL:
https://git.openjdk.org/jdk/commit/f7dbb98fe69eb98f8544577d81550b4fd817864b
Stats: 226 lines in 2 files changed: 213 ins; 1 del; 12 mod
8333086: Using Console.println is unnecessarily slow due to JLine initalization
Reviewed-by: asotona, naoto
-
PR: https://git.openjdk.org/jdk/pull/19479
Re: RFR: 8333086: Using Console.println is unnecessarily slow due to JLine initalization [v3]
> Consider these two programs:
>
>
> public class SystemPrint {
> public static void main(String... args) {
> System.err.println("Hello!");
> }
> }
>
> and:
>
> public class IOPrint {
> public static void main(String... args) {
> java.io.IO.println("Hello!");
> }
> }
>
>
> They do the same conceptual thing - write a text to the output. But,
> `IO.println` delegates to `Console.println`, which then delegates to a
> `Console` backend, and the default backend is currently based on JLine.
>
> The issues is that JLine takes a quite a long time to initialize, and in a
> program like this, JLine is not really needed - it is used to provide better
> editing experience when reading input, but there's no reading in these
> programs.
>
> For example, on my computer:
>
> $ time java -classpath /tmp SystemPrint
> Hello!
>
> real0m0,035s
> user0m0,019s
> sys 0m0,019s
>
> $ time java -classpath /tmp --enable-preview IOPrint
> Hello!
>
> real0m0,165s
> user0m0,324s
> sys 0m0,042s
>
>
> The proposal herein is to delegate to the simpler `Console` backend from
> `java.base` as long as the user only uses methods that print to output, and
> switch to the JLine delegate when other methods (typically input) is used.
> Note that while technically `writer()` is a method doing output, it will
> force JLine initialization to avoid possible problems if the client caches
> the writer and uses it after switching the delegates.
>
> With this patch, I can get timing like this:
>
> $ time java --enable-preview -classpath /tmp/ IOPrint
> Hello!
>
> real0m0,051s
> user0m0,038s
> sys 0m0,020s
>
>
> which seems much more acceptable.
>
> There is also #19467, which may reduce the time further.
>
> A future work might focus on making JLine initialize faster, but avoiding
> JLine initialization in case where we don't need it seems like a good step to
> me in any case.
Jan Lahoda has updated the pull request incrementally with one additional
commit since the last revision:
Correctly reflecting review feedback
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19479/files
- new: https://git.openjdk.org/jdk/pull/19479/files/7a0c448f..f3259793
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19479=02
- incr: https://webrevs.openjdk.org/?repo=jdk=19479=01-02
Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/19479.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19479/head:pull/19479
PR: https://git.openjdk.org/jdk/pull/19479
Re: RFR: 8333086: Using Console.println is unnecessarily slow due to JLine initalization [v2]
> Consider these two programs:
>
>
> public class SystemPrint {
> public static void main(String... args) {
> System.err.println("Hello!");
> }
> }
>
> and:
>
> public class IOPrint {
> public static void main(String... args) {
> java.io.IO.println("Hello!");
> }
> }
>
>
> They do the same conceptual thing - write a text to the output. But,
> `IO.println` delegates to `Console.println`, which then delegates to a
> `Console` backend, and the default backend is currently based on JLine.
>
> The issues is that JLine takes a quite a long time to initialize, and in a
> program like this, JLine is not really needed - it is used to provide better
> editing experience when reading input, but there's no reading in these
> programs.
>
> For example, on my computer:
>
> $ time java -classpath /tmp SystemPrint
> Hello!
>
> real0m0,035s
> user0m0,019s
> sys 0m0,019s
>
> $ time java -classpath /tmp --enable-preview IOPrint
> Hello!
>
> real0m0,165s
> user0m0,324s
> sys 0m0,042s
>
>
> The proposal herein is to delegate to the simpler `Console` backend from
> `java.base` as long as the user only uses methods that print to output, and
> switch to the JLine delegate when other methods (typically input) is used.
> Note that while technically `writer()` is a method doing output, it will
> force JLine initialization to avoid possible problems if the client caches
> the writer and uses it after switching the delegates.
>
> With this patch, I can get timing like this:
>
> $ time java --enable-preview -classpath /tmp/ IOPrint
> Hello!
>
> real0m0,051s
> user0m0,038s
> sys 0m0,020s
>
>
> which seems much more acceptable.
>
> There is also #19467, which may reduce the time further.
>
> A future work might focus on making JLine initialize faster, but avoiding
> JLine initialization in case where we don't need it seems like a good step to
> me in any case.
Jan Lahoda has updated the pull request incrementally with one additional
commit since the last revision:
Reflecting review feedback - explicitly selecting the jdk.internal.le
provider in the test.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19479/files
- new: https://git.openjdk.org/jdk/pull/19479/files/9886732e..7a0c448f
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19479=01
- incr: https://webrevs.openjdk.org/?repo=jdk=19479=00-01
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/19479.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19479/head:pull/19479
PR: https://git.openjdk.org/jdk/pull/19479
Re: RFR: 8330182: Start of release updates for JDK 24 [v9]
On Thu, 30 May 2024 18:39:19 GMT, Chen Liang wrote: >> Joe Darcy has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update symbol files for JDK 23 build 25. > > src/jdk.compiler/share/data/symbols/jdk.incubator.foreign-N.sym.txt line 1: > >> 1: # > > Just curious, does CreateSymbols not track module migrations, now that > jdk.incubator.foreign is completely merged into java.base? When writing these symbol files, CreateSymbols does not keep track where a given package was in a given version, and puts packages to files based on the first version where the package was first introduced. Technically, it should not matter, as the assignment of classes/packages to modules is not based on the file from which the description of the class was read. So, so far, it didn't seem necessary to keep track of package movement for writing of these files. But it is feasible to do so, if the need arises. (When writing ct.sym, it keeps track of the exact package module for every version, of course.) - PR Review Comment: https://git.openjdk.org/jdk/pull/18787#discussion_r1621320340
RFR: 8333086: Using Console.println is unnecessarily slow due to JLine initalization
Consider these two programs:
public class SystemPrint {
public static void main(String... args) {
System.err.println("Hello!");
}
}
and:
public class IOPrint {
public static void main(String... args) {
java.io.IO.println("Hello!");
}
}
They do the same conceptual thing - write a text to the output. But,
`IO.println` delegates to `Console.println`, which then delegates to a
`Console` backend, and the default backend is currently based on JLine.
The issues is that JLine takes a quite a long time to initialize, and in a
program like this, JLine is not really needed - it is used to provide better
editing experience when reading input, but there's no reading in these programs.
For example, on my computer:
$ time java -classpath /tmp SystemPrint
Hello!
real0m0,035s
user0m0,019s
sys 0m0,019s
$ time java -classpath /tmp --enable-preview IOPrint
Hello!
real0m0,165s
user0m0,324s
sys 0m0,042s
The proposal herein is to delegate to the simpler `Console` backend from
`java.base` as long as the user only uses methods that print to output, and
switch to the JLine delegate when other methods (typically input) is used. Note
that while technically `writer()` is a method doing output, it will force JLine
initialization to avoid possible problems if the client caches the writer and
uses it after switching the delegates.
With this patch, I can get timing like this:
$ time java --enable-preview -classpath /tmp/ IOPrint
Hello!
real0m0,051s
user0m0,038s
sys 0m0,020s
which seems much more acceptable.
There is also #19467, which may reduce the time further.
A future work might focus on making JLine initialize faster, but avoiding JLine
initialization in case where we don't need it seems like a good step to me in
any case.
-
Commit messages:
- Cleanup, addint test.
- Using println correctly, flushing the java.base delegate before switching to
JLine.
- Force Terminal when writer is requested.
- Attempting to speedup start by delaying initialization of JLine until really
necessary.
Changes: https://git.openjdk.org/jdk/pull/19479/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=19479=00
Issue: https://bugs.openjdk.org/browse/JDK-8333086
Stats: 225 lines in 2 files changed: 212 ins; 1 del; 12 mod
Patch: https://git.openjdk.org/jdk/pull/19479.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19479/head:pull/19479
PR: https://git.openjdk.org/jdk/pull/19479
Re: RFR: 8333103: Re-examine the console provider loading
On Wed, 29 May 2024 19:51:36 GMT, Naoto Sato wrote:
> There is an initialization code in `Console` class that searches for the
> Console implementations. Refactoring the init code not to use lambda/stream
> would reduce the (initial) number of loaded classes by about 100 for
> java.base implementations. This would become relevant when the java.io.IO
> (JEP 477) uses Console as the underlying framework.
Just to clarify a bit, this is only vaguely related to the implicitly declared
classes. The actual issue is that having two programs:
public class SystemPrint {
public static void main(String... args) {
System.err.println("Hello!");
}
}
and:
public class IOPrint {
public static void main(String... args) {
java.io.IO.println("Hello!");
}
}
(note there are no implicitly declared classes, and no implicit imports in the
samples), there is a considerable difference in the runtime of (compiled
versions) of these classes. E.g. on my laptop:
$ time java -classpath /tmp SystemPrint
Hello!
real0m0,035s
user0m0,019s
sys 0m0,019s
$ time java -classpath /tmp --enable-preview IOPrint
Hello!
real0m0,165s
user0m0,324s
sys 0m0,042s
(Vast) majority of the time is spent in JLine initialization. There's
https://bugs.openjdk.org/browse/JDK-8333086 for that, where the intent is to
avoid initializing JLine for simple printing. There may be other opportunities
to make JLine initialization faster in case we need to initialize it.
-
PR Comment: https://git.openjdk.org/jdk/pull/19467#issuecomment-2139255172
Re: RFR: 8332597: Remove redundant methods from j.l.classfile.ClassReader API [v2]
On Fri, 24 May 2024 16:41:33 GMT, Adam Sotona wrote: >> j.l.classfile.ClassReader instance is exposed in the Class-File API through >> j.l.classfile.AttributeMapper::readAttribute method only. >> ClassReader only purpose is to serve as a tool for reading content of a >> custom attribute in a user-provided AttribtueMapper. >> It contains useful set of low-level class reading methods for user to >> implement a custom attribute content parser. >> >> However methods ClassReader::thisClassPos and >> ClassReader::skipAttributeHolder are not necessary for a custom attribute >> content parsing and so redundant in the API. >> Class-File API implementation internally use these methods, however they >> should not be exposed in the API. >> >> This patch removes the methods from the API. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains two commits: > > - Merge branch 'master' into JDK-8332597-classreader-redundancy > ># Conflicts: ># src/java.base/share/classes/jdk/internal/classfile/impl/ClassImpl.java > - 8332597: Remove redundant methods from j.l.classfile.ClassReader API Marked as reviewed by jlahoda (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19323#pullrequestreview-2082655142
Re: RFR: 8332505: JEP 457: ClassRemapper forgets to remap bootstrap method references
On Mon, 20 May 2024 08:03:28 GMT, Adam Sotona wrote: > Class-File API `ClassRemapper` component suppose to remap all classes > referenced in a class file. > Actual implementation missed remapping of bootstrap methods referenced from > `invokedynamic` instructions. > This patch fixes the remapping and adds relevant test. > > Please review. > > Thanks, > Adam Looks reasonable to me. - Marked as reviewed by jlahoda (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19299#pullrequestreview-2082624013
Re: RFR: 8331051: Add an `@since` checker test for `java.base` module [v7]
On Wed, 22 May 2024 13:58:21 GMT, Nizar Benalla wrote:
>> This checker checks the values of the `@since` tag found in the
>> documentation comment for an element against the release in which the
>> element first appeared.
>>
>> Real since value of an API element is computed as the oldest release in
>> which the given API element was introduced. That is:
>> - for modules, classes and interfaces, the release in which the element with
>> the given qualified name was introduced
>> - for constructors, the release in which the constructor with the given VM
>> descriptor was introduced
>> - for methods and fields, the release in which the given method or field
>> with the given VM descriptor became a member of its enclosing class or
>> interface, whether direct or inherited
>>
>> Effective since value of an API element is computed as follows:
>> - if the given element has a `@since` tag in its javadoc, it is used
>> - in all other cases, return the effective since value of the enclosing
>> element
>>
>> The since checker verifies that for every API element, the real since value
>> and the effective since value are the same, and reports an error if they are
>> not.
>>
>> Preview method are handled as per JEP 12, if `@PreviewFeature` is used
>> consistently going forward then the checker doesn't need to be updated with
>> every release. The checker has explicit knowledge of preview elements that
>> came before `JDK 14` because they weren't marked in a machine understandable
>> way and preview elements that came before `JDK 17` that didn't have
>> `@PreviewFeature`.
>>
>> Important note : We only check code written since `JDK 9` as the releases
>> used to determine the expected value of `@since` tags are taken from the
>> historical data built into `javac` which only goes back that far
>>
>> The intial comment at the beginning of `SinceChecker.java` holds more
>> information into the program.
>>
>> I already have filed issues and fixed some wrong tags like in #18640,
>> #18032, #18030, #18055, #18373, #18954, #18972.
>
> Nizar Benalla has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add a few more legacy methods, needed a few more after changes to the
> checker
Looks good to me, thanks for doing this!
test/jdk/tools/sincechecker/SinceChecker.java line 351:
> 349: }
> 350: String uniqueId = getElementName(te, element, types);
> 351: boolean isCommonMethod = uniqueId.endsWith("toString()") ||
Nit: `.toString()` instead of `toString()`?
-
Marked as reviewed by jlahoda (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18934#pullrequestreview-2076973109
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1613507278
Re: RFR: 8305457: Implement java.io.IO [v14]
On Fri, 24 May 2024 11:30:19 GMT, Pavel Rappo wrote:
>> Please review this PR which introduces the `java.io.IO` top-level class and
>> three methods to `java.io.Console` for [Implicitly Declared Classes and
>> Instance Main Methods (Third Preview)].
>>
>> This PR has been obtained as `git merge --squash` of a now obsolete [draft
>> PR].
>>
>> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]:
>> https://bugs.openjdk.org/browse/JDK-8323335
>> [draft PR]: https://github.com/openjdk/jdk/pull/18921
>
> Pavel Rappo has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 26 commits:
>
> - Fix test failure
>
>Caused by 4e6d851f3f061b4a9c2b5d2e3fba6a0277ac1f34:
>
>8325324: Implement JEP 477: Implicitly Declared Classes
> and Instance Main Methods (Third Preview)
> - Merge branch 'master' into 8305457-Implement-java.io.IO
> - Merge remote-tracking branch 'jdk/master' into 8305457-Implement-java.io.IO
> - Merge branch 'master' into 8305457-Implement-java.io.IO
> - Force reasonable terminal size
>
>JLine outputs unexpected stuff if the terminal isn't dumb and small,
>such as that of our CI machines:
>
>if (newLines.size() > displaySize && !isTerminalDumb()) {
>StringBuilder sb = new StringBuilder(">");
>
>This causes the IO test to fail with timeout, because the expected
>prompt is never matched. To avoid that, we reasonably size the
>terminal.
> - Restructure the test
> - Add diagnostic output
> - Use "expect" that was found
> - Merge branch 'master' into 8305457-Implement-java.io.IO
>
># Conflicts:
># src/java.base/share/classes/java/io/ProxyingConsole.java
># src/java.base/share/classes/jdk/internal/io/JdkConsole.java
># src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java
>#
> src/jdk.internal.le/share/classes/jdk/internal/org/jline/JdkConsoleProviderImpl.java
># src/jdk.jshell/share/classes/jdk/jshell/execution/impl/ConsoleImpl.java
> - Escape prompt
> - ... and 16 more: https://git.openjdk.org/jdk/compare/239c1b33...5edf686d
Looks good to me.
-
Marked as reviewed by jlahoda (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19112#pullrequestreview-2076957513
Re: RFR: 8305457: Implement java.io.IO [v13]
On Thu, 23 May 2024 17:14:19 GMT, Pavel Rappo wrote:
>> Please review this PR which introduces the `java.io.IO` top-level class and
>> three methods to `java.io.Console` for [Implicitly Declared Classes and
>> Instance Main Methods (Third Preview)].
>>
>> This PR has been obtained as `git merge --squash` of a now obsolete [draft
>> PR].
>>
>> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]:
>> https://bugs.openjdk.org/browse/JDK-8323335
>> [draft PR]: https://github.com/openjdk/jdk/pull/18921
>
> Pavel Rappo has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 23 commits:
>
> - Merge branch 'master' into 8305457-Implement-java.io.IO
> - Force reasonable terminal size
>
>JLine outputs unexpected stuff if the terminal isn't dumb and small,
>such as that of our CI machines:
>
>if (newLines.size() > displaySize && !isTerminalDumb()) {
>StringBuilder sb = new StringBuilder(">");
>
>This causes the IO test to fail with timeout, because the expected
>prompt is never matched. To avoid that, we reasonably size the
>terminal.
> - Restructure the test
> - Add diagnostic output
> - Use "expect" that was found
> - Merge branch 'master' into 8305457-Implement-java.io.IO
>
># Conflicts:
># src/java.base/share/classes/java/io/ProxyingConsole.java
># src/java.base/share/classes/jdk/internal/io/JdkConsole.java
># src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java
>#
> src/jdk.internal.le/share/classes/jdk/internal/org/jline/JdkConsoleProviderImpl.java
># src/jdk.jshell/share/classes/jdk/jshell/execution/impl/ConsoleImpl.java
> - Escape prompt
> - Merge branch 'master' into 8305457-Implement-java.io.IO
> - Clarify input charset
> - Make IO final
> - ... and 13 more: https://git.openjdk.org/jdk/compare/e19a421c...258ce133
JShell-related changes look good to me.
-
Marked as reviewed by jlahoda (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19112#pullrequestreview-2076598544
Re: RFR: 8332528: Generate code in SwitchBootstraps.generateTypeSwitch that require fewer adaptations [v4]
On Thu, 23 May 2024 11:09:14 GMT, Claes Redestad wrote: >> We can fold the call to `Objects.checkIndex` into the code generated in >> generateTypeSwitchSkeleton instead of doing so by filtering the MH argument. >> This loads 9 less classes (of which 8 generated LFs and Species classes) on >> a minimal test, while being neutral on a throughput sanity test: >> >> >> Name Cnt Base Error Test Error Unit Change >> SwitchSanity.switchSum 15 8,162 ± 0,117 8,152 ± 0,131 ns/op 1,00x (p = >> 0,800 ) >> * = significant >> ``` >> >> A few additional optimizations includes moving some seldom used `findStatic` >> calls to a holder. All in all this means a reduction by 22M cycles to >> bootstrap a trivial switch expression on my M1. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove unused import, revert left-overs in MethodTypeDescImpl Looks good, thanks! - Marked as reviewed by jlahoda (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19307#pullrequestreview-2073701019
Re: RFR: 8332528: Generate code in SwitchBootstraps.generateTypeSwitch that require fewer adaptations [v3]
On Tue, 21 May 2024 09:01:32 GMT, Claes Redestad wrote: >> We can fold the call to `Objects.checkIndex` into the code generated in >> generateTypeSwitchSkeleton instead of doing so by filtering the MH argument. >> This loads 9 less classes (of which 8 generated LFs and Species classes) on >> a minimal test, while being neutral on a throughput sanity test: >> >> >> Name Cnt Base Error Test Error Unit Change >> SwitchSanity.switchSum 15 8,162 ± 0,117 8,152 ± 0,131 ns/op 1,00x (p = >> 0,800 ) >> * = significant >> ``` >> >> A few additional optimizations includes moving some seldom used `findStatic` >> calls to a holder. All in all this means a reduction by 22M cycles to >> bootstrap a trivial switch expression on my M1. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Add type switch to HelloClasslist Seems goo to me, with one nit related to `MethodTypeDescImpl`. src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 53: > 51: import java.lang.classfile.instruction.SwitchCase; > 52: > 53: import jdk.internal.constant.MethodTypeDescImpl; Nit - this import seems to be unused, and neither seem to be the changes to `MethodTypeDescImpl`. Is there some use missing? (OTOH, I like the `StaticHolders` for the stuff that relates to the enum-switch special case, for the time being.) - PR Review: https://git.openjdk.org/jdk/pull/19307#pullrequestreview-2073448995 PR Review Comment: https://git.openjdk.org/jdk/pull/19307#discussion_r1611368747
Re: RFR: 8331051: Add an `@since` checker test for `java.base` module [v5]
On Sun, 5 May 2024 15:22:15 GMT, Nizar Benalla wrote:
>> This checker checks the values of the `@since` tag found in the
>> documentation comment for an element against the release in which the
>> element first appeared.
>>
>> Real since value of an API element is computed as the oldest release in
>> which the given API element was introduced. That is:
>> - for modules, classes and interfaces, the release in which the element with
>> the given qualified name was introduced
>> - for constructors, the release in which the constructor with the given VM
>> descriptor was introduced
>> - for methods and fields, the release in which the given method or field
>> with the given VM descriptor became a member of its enclosing class or
>> interface, whether direct or inherited
>>
>> Effective since value of an API element is computed as follows:
>> - if the given element has a `@since` tag in its javadoc, it is used
>> - in all other cases, return the effective since value of the enclosing
>> element
>>
>> The since checker verifies that for every API element, the real since value
>> and the effective since value are the same, and reports an error if they are
>> not.
>>
>> Preview method are handled as per JEP 12, if `@PreviewFeature` is used
>> consistently going forward then the checker doesn't need to be updated with
>> every release. The checker has explicit knowledge of preview elements that
>> came before `JDK 14` because they weren't marked in a machine understandable
>> way and preview elements that came before `JDK 17` that didn't have
>> `@PreviewFeature`.
>>
>> Important note : We only check code written since `JDK 9` as the releases
>> used to determine the expected value of `@since` tags are taken from the
>> historical data built into `javac` which only goes back that far
>>
>> The intial comment at the beginning of `SinceChecker.java` holds more
>> information into the program.
>>
>> I already have filed issues and fixed some wrong tags like in #18640,
>> #18032, #18030, #18055, #18373, #18954, #18972.
>
> Nizar Benalla has updated the pull request incrementally with one additional
> commit since the last revision:
>
> - Not checking elements enclosed within a record, I doubt a record class
> will change after being created
> - Added a null check as `toString` can return an exception
Overall seems pretty good, great job!
A few comments inline, to make the code cleaner.
Thanks for working on this!
test/jdk/tools/sincechecker/SinceChecker.java line 224:
> 222: }
> 223:
> 224: return LEGACY_PREVIEW_METHODS.containsKey(currentVersion)
Nit: could probably be `LEGACY_PREVIEW_METHODS.getOrDefault(currentVersion,
Map.of()).contains(uniqueId)`
test/jdk/tools/sincechecker/SinceChecker.java line 309:
> 307: error("module-info.java not found or couldn't be opened
> AND this module has no unqualified exports");
> 308: } catch (NullPointerException e) {
> 309: error("module-info.java does not contain an `@since`");
Catching a NPE like this feels like a code smell to me. I assume it may happen
when `extractSinceVersionFromText(moduleInfoContent)` returns `null` - so just
store that in a variable, and check for `null` there.
test/jdk/tools/sincechecker/SinceChecker.java line 331:
> 329: error(ed.getPackage().getQualifiedName() + ":
> package-info.java not found or couldn't be opened");
> 330: } catch (NullPointerException e) {
> 331: error(ed.getPackage().getQualifiedName() + ":
> package-info.java does not contain an `@since`");
Please see the comment on NPE catching in `getModuleVersionFromFile`.
test/jdk/tools/sincechecker/SinceChecker.java line 352:
> 350: }
> 351: checkElement(te.getEnclosingElement(), te, types, javadocHelper,
> version, elementUtils);
> 352: if( te.getKind() == ElementKind.RECORD){
This seems a bit too broad. Ignoring all members of a record type may lead to
missed members. `Elements.getOrigin` may be unusable here, but I wonder what
exactly is the problem which this condition is trying to solve?
-
PR Review: https://git.openjdk.org/jdk/pull/18934#pullrequestreview-2063646309
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1605134919
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1605140507
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1605143505
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1605150261
Re: RFR: 8330276: Console methods with explicit Locale [v7]
On Wed, 15 May 2024 17:05:17 GMT, Naoto Sato wrote: >> Proposing new overloaded methods in `java.io.Console` class that explicitly >> take a `Locale` argument. Existing methods rely on the default locale, so >> the users won't be able to format their prompts/outputs in a certain locale >> explicitly. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reverted test workaround. Fixed JLine (backing out a questionable change) Looks good to me. - Marked as reviewed by jlahoda (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18923#pullrequestreview-2058627699
Integrated: 8331535: Incorrect prompt for Console.readLine
On Fri, 3 May 2024 10:11:02 GMT, Jan Lahoda wrote:
> When JLine reads a line, there may be a prompt provided. However, JLine will
> not interpret the prompt literally, it will handle `%` specially. As a
> consequence, doing:
>
> System.console().readLine("%%s");
>
>
> will not print `%s`, as first `String.format` is used, which will convert
> `%%s` to `%s`, and then JLine will interpret the `%`. The proposed solution
> is to duplicate the `%`, so that JLine will print it.
This pull request has now been integrated.
Changeset: 5a8df410
Author:Jan Lahoda
URL:
https://git.openjdk.org/jdk/commit/5a8df4106ac5386eb72e874dcadf2b18defe27d8
Stats: 296 lines in 6 files changed: 291 ins; 0 del; 5 mod
8331535: Incorrect prompt for Console.readLine
8331681: Test that jdk.internal.io.JdkConsole does not interpret prompts
Reviewed-by: naoto, asotona
-
PR: https://git.openjdk.org/jdk/pull/19081
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v6]
> When JLine reads a line, there may be a prompt provided. However, JLine will
> not interpret the prompt literally, it will handle `%` specially. As a
> consequence, doing:
>
> System.console().readLine("%%s");
>
>
> will not print `%s`, as first `String.format` is used, which will convert
> `%%s` to `%s`, and then JLine will interpret the `%`. The proposed solution
> is to duplicate the `%`, so that JLine will print it.
Jan Lahoda has updated the pull request incrementally with one additional
commit since the last revision:
Update test/jdk/java/io/Console/ConsolePromptTest.java
Co-authored-by: Adam Sotona <10807609+asot...@users.noreply.github.com>
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19081/files
- new: https://git.openjdk.org/jdk/pull/19081/files/da84810f..441bdad8
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19081=05
- incr: https://webrevs.openjdk.org/?repo=jdk=19081=04-05
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/19081.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19081/head:pull/19081
PR: https://git.openjdk.org/jdk/pull/19081
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v5]
> When JLine reads a line, there may be a prompt provided. However, JLine will
> not interpret the prompt literally, it will handle `%` specially. As a
> consequence, doing:
>
> System.console().readLine("%%s");
>
>
> will not print `%s`, as first `String.format` is used, which will convert
> `%%s` to `%s`, and then JLine will interpret the `%`. The proposed solution
> is to duplicate the `%`, so that JLine will print it.
Jan Lahoda has updated the pull request incrementally with one additional
commit since the last revision:
Fixing the '%' prompt problem for JShell tools' console.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19081/files
- new: https://git.openjdk.org/jdk/pull/19081/files/a138981e..da84810f
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19081=04
- incr: https://webrevs.openjdk.org/?repo=jdk=19081=03-04
Stats: 85 lines in 3 files changed: 82 ins; 0 del; 3 mod
Patch: https://git.openjdk.org/jdk/pull/19081.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19081/head:pull/19081
PR: https://git.openjdk.org/jdk/pull/19081
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v4]
> When JLine reads a line, there may be a prompt provided. However, JLine will
> not interpret the prompt literally, it will handle `%` specially. As a
> consequence, doing:
>
> System.console().readLine("%%s");
>
>
> will not print `%s`, as first `String.format` is used, which will convert
> `%%s` to `%s`, and then JLine will interpret the `%`. The proposed solution
> is to duplicate the `%`, so that JLine will print it.
Jan Lahoda has updated the pull request incrementally with one additional
commit since the last revision:
Adding another test run with -Djdk.console=java.base, as suggested.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19081/files
- new: https://git.openjdk.org/jdk/pull/19081/files/05592871..a138981e
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19081=03
- incr: https://webrevs.openjdk.org/?repo=jdk=19081=02-03
Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/19081.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19081/head:pull/19081
PR: https://git.openjdk.org/jdk/pull/19081
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v3]
> When JLine reads a line, there may be a prompt provided. However, JLine will
> not interpret the prompt literally, it will handle `%` specially. As a
> consequence, doing:
>
> System.console().readLine("%%s");
>
>
> will not print `%s`, as first `String.format` is used, which will convert
> `%%s` to `%s`, and then JLine will interpret the `%`. The proposed solution
> is to duplicate the `%`, so that JLine will print it.
Jan Lahoda has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains eight commits:
- Adjusting test as suggested.
- Merge branch 'master' into JDK-8331535
- 8331535: Incorrect prompt for Console.readLine
- Fixing test.
- Attempting to stabilize the test.
- Improving test to really test the redirect while stdin is connected to a
terminal.
- Fixing typo.
- 8330998: System.console() writes to stderr when stdout is redirected
-
Changes: https://git.openjdk.org/jdk/pull/19081/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=19081=02
Stats: 210 lines in 3 files changed: 208 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/19081.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19081/head:pull/19081
PR: https://git.openjdk.org/jdk/pull/19081
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v2]
On Fri, 3 May 2024 21:59:40 GMT, Pavel Rappo wrote: >>> Ideally, we should have separate tests that make sure that jdk.internal.le >>> is the default impl. >> >> We have a test that checks if `System.console()` returns the correct Console >> (or null) from the expected module >> (`test/jdk/java/io/Console/ModuleSelectionTest.java`) >> >>> We should also test this behaviour (i.e. % interpretation) on >>> jdk.internal.io Console impl. >> >> Yeah, that would be helpful. > >> We have a test that checks if `System.console()` returns the correct Console >> (or null) from the expected module >> (`test/jdk/java/io/Console/ModuleSelectionTest.java`) >> > > Good; then here we should indeed specify `-Djdk.console=jdk.internal.le`. > Initially, I suggested an alternative wording (i.e. "default"), but that's > inappropriate. After all, the test in question resides in > `test/jdk/jdk/internal/jline`. So it only makes sense that it tests that > concrete implementation. > >> Yeah, that would be helpful. >> > > I filed a bug: https://bugs.openjdk.org/browse/JDK-8331681 Thanks. I've added `-Djdk.console=jdk.internal.le`, and also added a test for the `java.base`'s console: `test/jdk/java/io/Console/ConsolePromptTest.java`. I can easily remove the latter if this is not a reasonable form. Or I can fix it as needed. Please let me know. Thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1591228737
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v2]
On Fri, 3 May 2024 11:20:52 GMT, Pavel Rappo wrote:
>> Jan Lahoda 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.
>
> src/jdk.internal.le/share/classes/jdk/internal/org/jline/JdkConsoleProviderImpl.java
> line 101:
>
>> 99: try {
>> 100: initJLineIfNeeded();
>> 101: return jline.readLine(fmt.formatted(args).replace("%",
>> "%%"));
>
> I understand that [JLine interprets `%` in a
> prompt](https://github.com/openjdk/jdk/blob/4ed38f5ad5f822ab948257ed39717ea919fd32ed/src/jdk.internal.le/share/classes/jdk/internal/org/jline/reader/impl/LineReaderImpl.java#L4050),
> but are the interpretation rules documented on JLine GitHub page or
> elsewhere?
There's a description in the javadoc:
https://www.javadoc.io/doc/org.jline/jline/latest/org/jline/reader/LineReader.html
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1591196087
Re: RFR: 8329420: Java 22 (and 23) launcher calls default constructor although main() is static [v4]
On Wed, 17 Apr 2024 09:20:24 GMT, Jan Lahoda wrote:
>> Consider code like:
>>
>> public class MainClass {
>> public MainClass() {
>> System.out.println("Constructor called!");
>> }
>> public static void main() {
>> System.out.println("main called!");
>> }
>> }
>>
>> and compile and run it, with preview enabled, like:
>>
>> $ javac /tmp/MainClass.java
>> $ java --enable-preview -classpath /tmp MainClass
>> Constructor called!
>> main called!
>>
>>
>> That is wrong, as the `main` method is static, and there is no need to
>> create a new instance of the class.
>>
>> The reason is that as launcher attempts to invoke the main method, it goes
>> in the following order: 1) static-main-with-args; 2)
>> instance-main-with-args; 3) static-main-without-args; 4)
>> instance-main-without-args. But, for the instance variants, the code first
>> creates a new instance of the given class, and only then attempts to lookup
>> the `main` method, and will pass to the next option when the `main` method
>> lookup fails. So, when invoking static-main-without-args, the current class
>> instance may be created for instance-main-with-args, which will then fail
>> due to the missing `main(String[])` method.
>>
>> The proposed solution to this problem is to simply first do a lookup for the
>> `main` method (skipping to the next variant when the given main method does
>> not exist, without instantiating the current class).
>>
>> There is also a relatively closely related problem: what happens when the
>> constructor throws an exception?
>>
>> public class MainClass {
>> public MainClass() {
>> if (true) throw new RuntimeException();
>> }
>> public void main() {
>> System.out.println("main called!");
>> }
>> }
>>
>>
>> when compiled an run, this produces no output whatsoever:
>>
>> $ javac /tmp/MainClass.java
>> $ java --enable-preview -classpath /tmp MainClass
>> $
>>
>>
>> This is because any exceptions thrown from the constructor are effectively
>> ignored, and the launcher will continue with the next variant. This seems
>> wrong - the exception should be printed for the user, like:
>>
>> $ java --enable-preview -classpath /tmp/ MainClass
>> Exception in thread "main" java.lang.RuntimeException
>> at MainClass.(MainClass.java:3)
>>
>>
>> This patch proposes to do that by not consuming the exceptions thrown from
>> the constructor, and stop the propagation to the next variant.
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixing typo (should not continue after an exception from a constructor).
Superseded by https://github.com/openjdk/jdk/pull/18786.
-
PR Comment: https://git.openjdk.org/jdk/pull/18753#issuecomment-2096278740
Withdrawn: 8329420: Java 22 (and 23) launcher calls default constructor although main() is static
On Fri, 12 Apr 2024 10:16:36 GMT, Jan Lahoda wrote:
> Consider code like:
>
> public class MainClass {
> public MainClass() {
> System.out.println("Constructor called!");
> }
> public static void main() {
> System.out.println("main called!");
> }
> }
>
> and compile and run it, with preview enabled, like:
>
> $ javac /tmp/MainClass.java
> $ java --enable-preview -classpath /tmp MainClass
> Constructor called!
> main called!
>
>
> That is wrong, as the `main` method is static, and there is no need to create
> a new instance of the class.
>
> The reason is that as launcher attempts to invoke the main method, it goes in
> the following order: 1) static-main-with-args; 2) instance-main-with-args; 3)
> static-main-without-args; 4) instance-main-without-args. But, for the
> instance variants, the code first creates a new instance of the given class,
> and only then attempts to lookup the `main` method, and will pass to the next
> option when the `main` method lookup fails. So, when invoking
> static-main-without-args, the current class instance may be created for
> instance-main-with-args, which will then fail due to the missing
> `main(String[])` method.
>
> The proposed solution to this problem is to simply first do a lookup for the
> `main` method (skipping to the next variant when the given main method does
> not exist, without instantiating the current class).
>
> There is also a relatively closely related problem: what happens when the
> constructor throws an exception?
>
> public class MainClass {
> public MainClass() {
> if (true) throw new RuntimeException();
> }
> public void main() {
> System.out.println("main called!");
> }
> }
>
>
> when compiled an run, this produces no output whatsoever:
>
> $ javac /tmp/MainClass.java
> $ java --enable-preview -classpath /tmp MainClass
> $
>
>
> This is because any exceptions thrown from the constructor are effectively
> ignored, and the launcher will continue with the next variant. This seems
> wrong - the exception should be printed for the user, like:
>
> $ java --enable-preview -classpath /tmp/ MainClass
> Exception in thread "main" java.lang.RuntimeException
> at MainClass.(MainClass.java:3)
>
>
> This patch proposes to do that by not consuming the exceptions thrown from
> the constructor, and stop the propagation to the next variant.
This pull request has been closed without being integrated.
-
PR: https://git.openjdk.org/jdk/pull/18753
Integrated: 8331708: jdk/internal/jline/RedirectedStdOut.java times-out on macosx-aarch64
On Mon, 6 May 2024 08:28:50 GMT, Jan Lahoda wrote: > When integrating: > https://github.com/openjdk/jdk/pull/18996 > > I've forgot to push one last commit which was stabilizing the test of Mac > OS/X. I am sorry for that. > > The test will create a pseudo terminal, and change the current process > stdin/stdout to write into the pty. But, there's nothing reading from the pty > on the other side. This mostly works, OK, but the `ProcessTools` will write a > debug log into `System.out` (which will write to stdout, which is the pty), > and that blocks on Mac OS/X, presumably because there's nothing reading from > the pty. > > This patch changes the `System.out` for the main process to a scratch value, > so no write to stdout/FD 1 is done, and no blocking should happen. (The > process' stdout/FD 1 should remain attached to the pty.) This pull request has now been integrated. Changeset: 15862a2f Author:Jan Lahoda URL: https://git.openjdk.org/jdk/commit/15862a2f116331b7f439619c3aa1b5965e737044 Stats: 8 lines in 1 file changed: 6 ins; 1 del; 1 mod 8331708: jdk/internal/jline/RedirectedStdOut.java times-out on macosx-aarch64 Reviewed-by: asotona - PR: https://git.openjdk.org/jdk/pull/19097
RFR: 8331708: jdk/internal/jline/RedirectedStdOut.java times-out on macosx-aarch64
When integrating: https://github.com/openjdk/jdk/pull/18996 I've forgot to push one last commit which was stabilizing the test of Mac OS/X. I am sorry for that. The test will create a pseudo terminal, and change the current process stdin/stdout to write into the pty. But, there's nothing reading from the pty on the other side. This mostly works, OK, but the `ProcessTools` will write a debug log into `System.out` (which will write to stdout, which is the pty), and that blocks on Mac OS/X, presumably because there's nothing reading from the pty. This patch changes the `System.out` for the main process to a scratch value, so no write to stdout/FD 1 is done, and no blocking should happen. (The process' stdout/FD 1 should remain attached to the pty.) - Commit messages: - 8331708: jdk/internal/jline/RedirectedStdOut.java times-out on macosx-aarch64 Changes: https://git.openjdk.org/jdk/pull/19097/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19097=00 Issue: https://bugs.openjdk.org/browse/JDK-8331708 Stats: 8 lines in 1 file changed: 6 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19097.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19097/head:pull/19097 PR: https://git.openjdk.org/jdk/pull/19097
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v2]
> When JLine reads a line, there may be a prompt provided. However, JLine will
> not interpret the prompt literally, it will handle `%` specially. As a
> consequence, doing:
>
> System.console().readLine("%%s");
>
>
> will not print `%s`, as first `String.format` is used, which will convert
> `%%s` to `%s`, and then JLine will interpret the `%`. The proposed solution
> is to duplicate the `%`, so that JLine will print it.
Jan Lahoda 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.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19081/files
- new: https://git.openjdk.org/jdk/pull/19081/files/58cbd42e..58cbd42e
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19081=01
- incr: https://webrevs.openjdk.org/?repo=jdk=19081=00-01
Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/19081.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19081/head:pull/19081
PR: https://git.openjdk.org/jdk/pull/19081
Integrated: 8330998: System.console() writes to stderr when stdout is redirected
On Mon, 29 Apr 2024 11:44:50 GMT, Jan Lahoda wrote:
> Consider code like:
>
> public class ConsoleTest {
> public static void main(String... args) {
> System.console().printf("Hello!");
> }
> }
>
>
> When run as:
>
> $ java ConsoleTest.java >/dev/null
>
>
> it prints `Hello!` to stderr, instead of to stdout (where it would be
> redirected).
>
> The proposed fix is to simply force the use of stdout. Sadly, this cannot be
> done solely using JLine configuration, we actually need to change the JLine's
> code for that.
>
> The most tricky part is a test. There are two sub-tests, one effectively
> testing a case where all of stdin/out/err are redirected, the other is
> attempting to test the case where stdin is attached to a terminal, while
> stdout is redirected. The second sub-test using a native functions to create
> a pty and to attach to it, and should run in a separate VM, as it leaves the
> VM attached to the terminal.
This pull request has now been integrated.
Changeset: f1509e00
Author:Jan Lahoda
URL:
https://git.openjdk.org/jdk/commit/f1509e007d1538acfb1749f7fafc56be2affd2e6
Stats: 183 lines in 3 files changed: 180 ins; 0 del; 3 mod
8330998: System.console() writes to stderr when stdout is redirected
Reviewed-by: naoto
-
PR: https://git.openjdk.org/jdk/pull/18996
Integrated: 8328481: Implement JEP 476: Module Import Declarations (Preview)
On Thu, 4 Apr 2024 07:30:34 GMT, Jan Lahoda wrote: > This is an implementation of JEP JDK-8315129: Module Import Declarations > (Preview). Please see the JEP for details: > https://bugs.openjdk.org/browse/JDK-8315129 > > It is mostly straightforward - the module imports are parsed, and then > expanded to import-on-demand in `TypeEnter`. > There is a few notable aspects, however: > - the AST node for import (`JCImport`) is holding the imported element as a > field access, because so far, the imported element always had to have a '.' > (even for import-on-demand). But for module imports, it is permissible to > import from a module whose name does not have a dot (`import module m;`). The > use of field access for ordinary import seems very useful, so I preferred to > keep that, and created a new internal-only AST node for module imports. There > is still only one public API AST node/interface, so this is purely an > implementation choice. > - JShell now supports module imports as well; and the default, implicit, > script is changed to use it to import all of `java.base` if preview is > enabled. It is expected that the default would be changed if/when the module > imports feature is finalized. This pull request has now been integrated. Changeset: f2c4a413 Author:Jan Lahoda URL: https://git.openjdk.org/jdk/commit/f2c4a41304d4fe984b79792cb3be460d7026e812 Stats: 1343 lines in 31 files changed: 1268 ins; 17 del; 58 mod 8328481: Implement JEP 476: Module Import Declarations (Preview) Co-authored-by: Jim Laskey Reviewed-by: mcimadamore, vromero - PR: https://git.openjdk.org/jdk/pull/18614
RFR: 8331535: Incorrect prompt for Console.readLine
When JLine reads a line, there may be a prompt provided. However, JLine will
not interpret the prompt literally, it will handle `%` specially. As a
consequence, doing:
System.console().readLine("%%s");
will not print `%s`, as first `String.format` is used, which will convert `%%s`
to `%s`, and then JLine will interpret the `%`. The proposed solution is to
duplicate the `%`, so that JLine will print it.
-
Depends on: https://git.openjdk.org/jdk/pull/18996
Commit messages:
- 8331535: Incorrect prompt for Console.readLine
Changes: https://git.openjdk.org/jdk/pull/19081/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=19081=00
Issue: https://bugs.openjdk.org/browse/JDK-8331535
Stats: 106 lines in 2 files changed: 104 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/19081.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19081/head:pull/19081
PR: https://git.openjdk.org/jdk/pull/19081
Re: RFR: 8331582: Incorrect default Console provider comment
On Thu, 2 May 2024 20:54:58 GMT, Naoto Sato wrote: > Fixing a comment in `java.io.Console`, which was a leftover from the fix to > https://bugs.openjdk.org/browse/JDK-8308591 Looks good to me, thanks! - Marked as reviewed by jlahoda (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19070#pullrequestreview-2037481621
Re: RFR: 8330998: System.console() writes to stderr when stdout is redirected [v2]
On Mon, 29 Apr 2024 18:41:44 GMT, Naoto Sato wrote: > Looks good to me. Left some minor suggestions. BTW, should we file an issue > at the `JLine` project, not to redirect to stderr, or suggest a new config > (sorry if it has already been taken care of)? The code to choose the output stream is quite elaborate, so I assume that is intentional. For new versions of JLine, the `systemOutput(SystemOutput.SysOut)` forces the use of stdout, however, so the patch to JLine itself will not be needed anymore after we upgrade. - PR Comment: https://git.openjdk.org/jdk/pull/18996#issuecomment-2088894660
Re: RFR: 8328481: Implement Module Imports [v13]
> This is an implementation of JEP JDK-8315129: Module Import Declarations > (Preview). Please see the JEP for details: > https://bugs.openjdk.org/browse/JDK-8315129 > > It is mostly straightforward - the module imports are parsed, and then > expanded to import-on-demand in `TypeEnter`. > There is a few notable aspects, however: > - the AST node for import (`JCImport`) is holding the imported element as a > field access, because so far, the imported element always had to have a '.' > (even for import-on-demand). But for module imports, it is permissible to > import from a module whose name does not have a dot (`import module m;`). The > use of field access for ordinary import seems very useful, so I preferred to > keep that, and created a new internal-only AST node for module imports. There > is still only one public API AST node/interface, so this is purely an > implementation choice. > - JShell now supports module imports as well; and the default, implicit, > script is changed to use it to import all of `java.base` if preview is > enabled. It is expected that the default would be changed if/when the module > imports feature is finalized. Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 28 commits: - Merge branch 'master' into module-imports - Fixing test on Windows (2). - Fixing Imports test on Windows. - Adding test for ImportTree.isModule, as suggested. - Reflecting review feedback - improving the 'module (current) does not read (target). - Updating JEP number and caption. - Fixing ListModuleDeps test. - Cleanup. - Merge branch 'master' into module-imports - Including current module name as suggested. - ... and 18 more: https://git.openjdk.org/jdk/compare/4e5c25ee...43a1e0f6 - Changes: https://git.openjdk.org/jdk/pull/18614/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18614=12 Stats: 1343 lines in 31 files changed: 1268 ins; 17 del; 58 mod Patch: https://git.openjdk.org/jdk/pull/18614.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18614/head:pull/18614 PR: https://git.openjdk.org/jdk/pull/18614
Re: RFR: 8330998: System.console() writes to stderr when stdout is redirected [v3]
> Consider code like:
>
> public class ConsoleTest {
> public static void main(String... args) {
> System.console().printf("Hello!");
> }
> }
>
>
> When run as:
>
> $ java ConsoleTest.java >/dev/null
>
>
> it prints `Hello!` to stderr, instead of to stdout (where it would be
> redirected).
>
> The proposed fix is to simply force the use of stdout. Sadly, this cannot be
> done solely using JLine configuration, we actually need to change the JLine's
> code for that.
>
> The most tricky part is a test. There are two sub-tests, one effectively
> testing a case where all of stdin/out/err are redirected, the other is
> attempting to test the case where stdin is attached to a terminal, while
> stdout is redirected. The second sub-test using a native functions to create
> a pty and to attach to it, and should run in a separate VM, as it leaves the
> VM attached to the terminal.
Jan Lahoda has updated the pull request incrementally with one additional
commit since the last revision:
Adjusting test, as suggested.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18996/files
- new: https://git.openjdk.org/jdk/pull/18996/files/8a918e3f..76599ac9
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18996=02
- incr: https://webrevs.openjdk.org/?repo=jdk=18996=01-02
Stats: 57 lines in 1 file changed: 6 ins; 35 del; 16 mod
Patch: https://git.openjdk.org/jdk/pull/18996.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18996/head:pull/18996
PR: https://git.openjdk.org/jdk/pull/18996
Re: RFR: 8330998: System.console() writes to stderr when stdout is redirected [v2]
On Mon, 29 Apr 2024 20:21:23 GMT, Archie Cobbs wrote: > > Of course the question is if it should write to stderr or /dev/tty like > > mechanism.. > > I was wondering the same thing. My understanding of the definition of > "console" is a bidirectional byte channel with a keyboard & screen on the > other end. It has no concept of stdout vs. stderr. It just has a display (or > in the old days, a printer). > > The function of a "shell" is to intermediate between one or more executing > programs and the console. It figures out how & when to actually display on > the console any stuff written to stdout and/or stderr by one of the programs > it has launched. > > A program can also access its console (if any) directly by opening /dev/tty > or whatever, thereby bypassing the shell. > > So I would think writing to something called "System.console()" from Java > (which is a program) would have nothing to do with Java's stderr or stdout, > except for a possible downstream interleaving with what the shell may also be > writing to the console at the same time. I tried a few programs, like `mc`, `top` and `htop`. When I redirect the stdout for them, they still write into it (and there's obviously nothing on the terminal, and there are escape sequences in the target file of the redirect). The only program I know from the top of my head that (AFAIK) bypasses stdin/stdout and reaches directly to the controlling terminal is `ssh` when reading passwords (for quite obvious very special reasons). I.e. not trying to be too smart about output, and simply using stdout as other programs do seems consistent, and most useful - the output can then be used in pipes, etc. - PR Comment: https://git.openjdk.org/jdk/pull/18996#issuecomment-2085419986
Re: RFR: 8330998: System.console() writes to stderr when stdout is redirected [v2]
> Consider code like:
>
> public class ConsoleTest {
> public static void main(String... args) {
> System.console().printf("Hello!");
> }
> }
>
>
> When run as:
>
> $ java ConsoleTest.java >/dev/null
>
>
> it prints `Hello!` to stderr, instead of to stdout (where it would be
> redirected).
>
> The proposed fix is to simply force the use of stdout. Sadly, this cannot be
> done solely using JLine configuration, we actually need to change the JLine's
> code for that.
>
> The most tricky part is a test. There are two sub-tests, one effectively
> testing a case where all of stdin/out/err are redirected, the other is
> attempting to test the case where stdin is attached to a terminal, while
> stdout is redirected. The second sub-test using a native functions to create
> a pty and to attach to it, and should run in a separate VM, as it leaves the
> VM attached to the terminal.
Jan Lahoda 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 six additional commits since the
last revision:
- Merge branch 'master' into JDK-8330998
- Fixing test.
- Attempting to stabilize the test.
- Improving test to really test the redirect while stdin is connected to a
terminal.
- Fixing typo.
- 8330998: System.console() writes to stderr when stdout is redirected
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18996/files
- new: https://git.openjdk.org/jdk/pull/18996/files/9090e4c7..8a918e3f
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18996=01
- incr: https://webrevs.openjdk.org/?repo=jdk=18996=00-01
Stats: 22740 lines in 1516 files changed: 8365 ins; 10464 del; 3911 mod
Patch: https://git.openjdk.org/jdk/pull/18996.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18996/head:pull/18996
PR: https://git.openjdk.org/jdk/pull/18996
Re: RFR: 8320575: generic type information lost on mandated parameters of record's compact constructors [v17]
On Fri, 26 Apr 2024 23:54:17 GMT, Vicente Romero wrote: >> Reflection is not retrieving generic type information for mandated >> parameters. This is a known issue which has been explicitly stated in the >> API of some reflection methods. Fix for >> [JDK-8292275](https://bugs.openjdk.org/browse/JDK-8292275) made the >> parameters of compact constructors of record classes `mandated` as specified >> in the spec. But this implied that users that previous to this change could >> retrieve the generic type of parameters of compact constructors now they >> can't anymore. The proposed fix is to try to retrieve generic type >> information for mandated parameters if available plus changing the spec of >> the related reflection methods. >> >> TIA > > Vicente Romero has updated the pull request incrementally with one additional > commit since the last revision: > > code refactoring Makes sense to me. Moving the variable declarations to the else section may improve the code as suggest by @liach. Getting a review from somebody knowledgeable about the reflection might be good. - Marked as reviewed by jlahoda (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17070#pullrequestreview-2028320315
RFR: 8330998: System.console() writes to stderr when stdout is redirected
Consider code like:
public class ConsoleTest {
public static void main(String... args) {
System.console().printf("Hello!");
}
}
When run as:
$ java ConsoleTest.java >/dev/null
it prints `Hello!` to stderr, instead of to stdout (where it would be
redirected).
The proposed fix is to simply force the use of stdout. Sadly, this cannot be
done solely using JLine configuration, we actually need to change the JLine's
code for that.
The most tricky part is a test. There are two sub-tests, one effectively
testing a case where all of stdin/out/err are redirected, the other is
attempting to test the case where stdin is attached to a terminal, while stdout
is redirected. The second sub-test using a native functions to create a pty and
to attach to it, and should run in a separate VM, as it leaves the VM attached
to the terminal.
-
Commit messages:
- Fixing test.
- Attempting to stabilize the test.
- Improving test to really test the redirect while stdin is connected to a
terminal.
- Fixing typo.
- 8330998: System.console() writes to stderr when stdout is redirected
Changes: https://git.openjdk.org/jdk/pull/18996/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=18996=00
Issue: https://bugs.openjdk.org/browse/JDK-8330998
Stats: 212 lines in 3 files changed: 209 ins; 0 del; 3 mod
Patch: https://git.openjdk.org/jdk/pull/18996.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18996/head:pull/18996
PR: https://git.openjdk.org/jdk/pull/18996
Re: RFR: 8324651: Compiler Implementation for Derived Record Creation (Preview) [v12]
> This is a patch for javac, that adds the Derived Record Creation expressions. > The current draft specification for the feature is: > https://cr.openjdk.org/~gbierman/jep468/jep468-20240326/specs/derived-record-creation-jls.html > > The current CSR is here: > https://bugs.openjdk.org/browse/JDK-8328637 > > The patch is mostly straightforward, with two notable changes: > - there is a new `ElementKind.COMPONENT_LOCAL_VARIABLE`, as the > specification introduces this term, and it seems consistent with > `ElementKind.BINDING_VARIABLE` that was introduced some time ago. > - there are a bit broader changes in `Flow`, to facilitate the introduction > of variables without an explicit declaration for definite assignment and > effectively final computation. Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 36 commits: - Merge branch 'master' into wthexp - Post-merge fixes. - Merge branch 'master' into wthexp - Reflecting review feedback: - reverting unnecessary changes in TransPatterns - moving the patters/withers/Model test to a more appropriate place - Adding tests as suggested. - Reflecting review feedback: - pre-generating the JCVarDecls in Attr, to aid Flow - adding a note on how the desugared code looks like - JavaCompiler cleanup - Improving the javax.lang.model for component local variables, by darcy. - Reflecting review feedback. - Merge branch 'master' into wthexp - ... and 26 more: https://git.openjdk.org/jdk/compare/4e5c25ee...c97eb8e6 - Changes: https://git.openjdk.org/jdk/pull/18509/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18509=11 Stats: 2023 lines in 51 files changed: 1965 ins; 19 del; 39 mod Patch: https://git.openjdk.org/jdk/pull/18509.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18509/head:pull/18509 PR: https://git.openjdk.org/jdk/pull/18509
Re: RFR: 8328481: Implement Module Imports [v12]
> This is an implementation of JEP JDK-8315129: Module Import Declarations > (Preview). Please see the JEP for details: > https://bugs.openjdk.org/browse/JDK-8315129 > > It is mostly straightforward - the module imports are parsed, and then > expanded to import-on-demand in `TypeEnter`. > There is a few notable aspects, however: > - the AST node for import (`JCImport`) is holding the imported element as a > field access, because so far, the imported element always had to have a '.' > (even for import-on-demand). But for module imports, it is permissible to > import from a module whose name does not have a dot (`import module m;`). The > use of field access for ordinary import seems very useful, so I preferred to > keep that, and created a new internal-only AST node for module imports. There > is still only one public API AST node/interface, so this is purely an > implementation choice. > - JShell now supports module imports as well; and the default, implicit, > script is changed to use it to import all of `java.base` if preview is > enabled. It is expected that the default would be changed if/when the module > imports feature is finalized. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Fixing Imports test on Windows. - Changes: - all: https://git.openjdk.org/jdk/pull/18614/files - new: https://git.openjdk.org/jdk/pull/18614/files/cbc363ab..1a621447 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18614=11 - incr: https://webrevs.openjdk.org/?repo=jdk=18614=10-11 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18614.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18614/head:pull/18614 PR: https://git.openjdk.org/jdk/pull/18614
Re: RFR: 8324651: Compiler Implementation for Derived Record Creation (Preview) [v11]
> This is a patch for javac, that adds the Derived Record Creation expressions. > The current draft specification for the feature is: > https://cr.openjdk.org/~gbierman/jep468/jep468-20240326/specs/derived-record-creation-jls.html > > The current CSR is here: > https://bugs.openjdk.org/browse/JDK-8328637 > > The patch is mostly straightforward, with two notable changes: > - there is a new `ElementKind.COMPONENT_LOCAL_VARIABLE`, as the > specification introduces this term, and it seems consistent with > `ElementKind.BINDING_VARIABLE` that was introduced some time ago. > - there are a bit broader changes in `Flow`, to facilitate the introduction > of variables without an explicit declaration for definite assignment and > effectively final computation. Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 35 commits: - Post-merge fixes. - Merge branch 'master' into wthexp - Reflecting review feedback: - reverting unnecessary changes in TransPatterns - moving the patters/withers/Model test to a more appropriate place - Adding tests as suggested. - Reflecting review feedback: - pre-generating the JCVarDecls in Attr, to aid Flow - adding a note on how the desugared code looks like - JavaCompiler cleanup - Improving the javax.lang.model for component local variables, by darcy. - Reflecting review feedback. - Merge branch 'master' into wthexp - Fixing tests. - ... and 25 more: https://git.openjdk.org/jdk/compare/177092b9...e8499df1 - Changes: https://git.openjdk.org/jdk/pull/18509/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18509=10 Stats: 2023 lines in 51 files changed: 1965 ins; 19 del; 39 mod Patch: https://git.openjdk.org/jdk/pull/18509.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18509/head:pull/18509 PR: https://git.openjdk.org/jdk/pull/18509
Re: RFR: 8330681: Explicit hashCode and equals for java.lang.runtime.SwitchBootstraps$TypePairs
On Fri, 19 Apr 2024 13:23:53 GMT, Claes Redestad wrote: > We can reduce overhead of first use of a switch bootstrap by moving > `typePairToName` into `TypePairs` and by explicitly overriding `hashCode` and > `equals`. The first change avoids loading and initializing the `TypePairs` > class in actual cases, the second remove some excess code generation from > happening on first use. Looks good to me. - Marked as reviewed by jlahoda (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18865#pullrequestreview-2011427815
Re: RFR: 8328481: Implement Module Imports [v11]
> This is an implementation of JEP JDK-8315129: Module Import Declarations > (Preview). Please see the JEP for details: > https://bugs.openjdk.org/browse/JDK-8315129 > > It is mostly straightforward - the module imports are parsed, and then > expanded to import-on-demand in `TypeEnter`. > There is a few notable aspects, however: > - the AST node for import (`JCImport`) is holding the imported element as a > field access, because so far, the imported element always had to have a '.' > (even for import-on-demand). But for module imports, it is permissible to > import from a module whose name does not have a dot (`import module m;`). The > use of field access for ordinary import seems very useful, so I preferred to > keep that, and created a new internal-only AST node for module imports. There > is still only one public API AST node/interface, so this is purely an > implementation choice. > - JShell now supports module imports as well; and the default, implicit, > script is changed to use it to import all of `java.base` if preview is > enabled. It is expected that the default would be changed if/when the module > imports feature is finalized. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Adding test for ImportTree.isModule, as suggested. - Changes: - all: https://git.openjdk.org/jdk/pull/18614/files - new: https://git.openjdk.org/jdk/pull/18614/files/846b038e..cbc363ab Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18614=10 - incr: https://webrevs.openjdk.org/?repo=jdk=18614=09-10 Stats: 135 lines in 1 file changed: 135 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18614.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18614/head:pull/18614 PR: https://git.openjdk.org/jdk/pull/18614
Re: RFR: 8328481: Implement Module Imports [v10]
> This is an implementation of JEP JDK-8315129: Module Import Declarations > (Preview). Please see the JEP for details: > https://bugs.openjdk.org/browse/JDK-8315129 > > It is mostly straightforward - the module imports are parsed, and then > expanded to import-on-demand in `TypeEnter`. > There is a few notable aspects, however: > - the AST node for import (`JCImport`) is holding the imported element as a > field access, because so far, the imported element always had to have a '.' > (even for import-on-demand). But for module imports, it is permissible to > import from a module whose name does not have a dot (`import module m;`). The > use of field access for ordinary import seems very useful, so I preferred to > keep that, and created a new internal-only AST node for module imports. There > is still only one public API AST node/interface, so this is purely an > implementation choice. > - JShell now supports module imports as well; and the default, implicit, > script is changed to use it to import all of `java.base` if preview is > enabled. It is expected that the default would be changed if/when the module > imports feature is finalized. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Reflecting review feedback - improving the 'module (current) does not read (target). - Changes: - all: https://git.openjdk.org/jdk/pull/18614/files - new: https://git.openjdk.org/jdk/pull/18614/files/432393ab..846b038e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18614=09 - incr: https://webrevs.openjdk.org/?repo=jdk=18614=08-09 Stats: 4 lines in 3 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/18614.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18614/head:pull/18614 PR: https://git.openjdk.org/jdk/pull/18614
Re: RFR: 8328481: Implement Module Imports [v9]
On Thu, 18 Apr 2024 10:47:11 GMT, Maurizio Cimadamore
wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Updating JEP number and caption.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties
> line 3541:
>
>> 3539: # 0: symbol, 1: symbol
>> 3540: compiler.err.import.module.does.not.read=\
>> 3541: {1} module does not read: {0}
>
> shouldn't it be `module {1} does not read: {0}` ? Also, maybe worth
> reordering the params?
Thanks! Fixed:
https://github.com/openjdk/jdk/pull/18614/commits/846b038ed8145ede9c419daddedb794a429dafac
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18614#discussion_r1570597292
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Mon, 15 Apr 2024 18:25:02 GMT, Sonia Zaldana Calles wrote: > Hi folks, > > This PR aims to fix > [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581). > > I think the regression got introduced in > [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458). > > In the issue linked above, > [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226) > got removed to simplify launcher code. > > Previously, we used ```getMainType``` to do the appropriate main method > invocation in ```JavaMain```. However, we currently attempt to do all types > of main method invocations at the same time > [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623). > > > Note how all of these invocations clear the exception reported with > [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390). > > > Therefore, if a legitimate exception comes up during one of these > invocations, it does not get reported. > > I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking > forward to your suggestions. > > Cheers, > Sonia My personal comments here: - I am fine with a solution like this. In 18753, I wanted to avoid a change of dynamics between the Java helper and the native part. But if we can change that, it looks better. I would suggest to take the test from 18753 though - doing a change like this without a test may lead to hard-to-find regressions in the future. (Note the current test should guard against both JDK-8329420 and JDK-8329581.) Or write a different test. - as Mandy points out, `LaucherHelper` already reads/has variables for "is-static" and "no-arguments" in `validateMainMethod`, so it should be possible to just use that values; also as Mandy points out, we can probably get rid of `CHECK_EXCEPTION_FAIL` and `CHECK_EXCEPTION_NULL_FAIL`, and use the `..._LEAVE` variants, no? (The `..._FAIL` variants where needed so that the launcher could continue with the next variant, but since we now only call the correct variant, we can just stop if something goes wrong?) - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2063217552
Re: RFR: 8328481: Implement Module Imports [v8]
On Thu, 18 Apr 2024 05:43:03 GMT, Alan Bateman wrote: >> Jan Lahoda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixing ListModuleDeps test. > > src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java line 84: > >> 82: @JEP(number=461, title="Stream Gatherers", status="Preview") >> 83: STREAM_GATHERERS, >> 84: @JEP(number=0, title="Module Imports", status="Preview") > > I see this has been assigned JEP 476 so I assume you can set the number for > the first integration. Done, thanks: https://github.com/openjdk/jdk/pull/18614/commits/432393abb4ac1f5c4730d982a3911284fe866318 - PR Review Comment: https://git.openjdk.org/jdk/pull/18614#discussion_r1570071215
Re: RFR: 8328481: Implement Module Imports [v9]
> This is an implementation of JEP JDK-8315129: Module Import Declarations > (Preview). Please see the JEP for details: > https://bugs.openjdk.org/browse/JDK-8315129 > > It is mostly straightforward - the module imports are parsed, and then > expanded to import-on-demand in `TypeEnter`. > There is a few notable aspects, however: > - the AST node for import (`JCImport`) is holding the imported element as a > field access, because so far, the imported element always had to have a '.' > (even for import-on-demand). But for module imports, it is permissible to > import from a module whose name does not have a dot (`import module m;`). The > use of field access for ordinary import seems very useful, so I preferred to > keep that, and created a new internal-only AST node for module imports. There > is still only one public API AST node/interface, so this is purely an > implementation choice. > - JShell now supports module imports as well; and the default, implicit, > script is changed to use it to import all of `java.base` if preview is > enabled. It is expected that the default would be changed if/when the module > imports feature is finalized. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Updating JEP number and caption. - Changes: - all: https://git.openjdk.org/jdk/pull/18614/files - new: https://git.openjdk.org/jdk/pull/18614/files/7fa0ad51..432393ab Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18614=08 - incr: https://webrevs.openjdk.org/?repo=jdk=18614=07-08 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18614.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18614/head:pull/18614 PR: https://git.openjdk.org/jdk/pull/18614
Re: RFR: 8328481: Implement Module Imports [v8]
> This is an implementation of JEP JDK-8315129: Module Import Declarations > (Preview). Please see the JEP for details: > https://bugs.openjdk.org/browse/JDK-8315129 > > It is mostly straightforward - the module imports are parsed, and then > expanded to import-on-demand in `TypeEnter`. > There is a few notable aspects, however: > - the AST node for import (`JCImport`) is holding the imported element as a > field access, because so far, the imported element always had to have a '.' > (even for import-on-demand). But for module imports, it is permissible to > import from a module whose name does not have a dot (`import module m;`). The > use of field access for ordinary import seems very useful, so I preferred to > keep that, and created a new internal-only AST node for module imports. There > is still only one public API AST node/interface, so this is purely an > implementation choice. > - JShell now supports module imports as well; and the default, implicit, > script is changed to use it to import all of `java.base` if preview is > enabled. It is expected that the default would be changed if/when the module > imports feature is finalized. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Fixing ListModuleDeps test. - Changes: - all: https://git.openjdk.org/jdk/pull/18614/files - new: https://git.openjdk.org/jdk/pull/18614/files/27f9cfb5..7fa0ad51 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18614=07 - incr: https://webrevs.openjdk.org/?repo=jdk=18614=06-07 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18614.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18614/head:pull/18614 PR: https://git.openjdk.org/jdk/pull/18614
Re: RFR: 8328481: Implement Module Imports [v7]
> This is an implementation of JEP JDK-8315129: Module Import Declarations > (Preview). Please see the JEP for details: > https://bugs.openjdk.org/browse/JDK-8315129 > > It is mostly straightforward - the module imports are parsed, and then > expanded to import-on-demand in `TypeEnter`. > There is a few notable aspects, however: > - the AST node for import (`JCImport`) is holding the imported element as a > field access, because so far, the imported element always had to have a '.' > (even for import-on-demand). But for module imports, it is permissible to > import from a module whose name does not have a dot (`import module m;`). The > use of field access for ordinary import seems very useful, so I preferred to > keep that, and created a new internal-only AST node for module imports. There > is still only one public API AST node/interface, so this is purely an > implementation choice. > - JShell now supports module imports as well; and the default, implicit, > script is changed to use it to import all of `java.base` if preview is > enabled. It is expected that the default would be changed if/when the module > imports feature is finalized. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Cleanup. - Changes: - all: https://git.openjdk.org/jdk/pull/18614/files - new: https://git.openjdk.org/jdk/pull/18614/files/c75b8b7a..27f9cfb5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18614=06 - incr: https://webrevs.openjdk.org/?repo=jdk=18614=05-06 Stats: 8 lines in 1 file changed: 0 ins; 8 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18614.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18614/head:pull/18614 PR: https://git.openjdk.org/jdk/pull/18614
Re: RFR: 8328481: Implement Module Imports [v6]
> This is an implementation of JEP JDK-8315129: Module Import Declarations > (Preview). Please see the JEP for details: > https://bugs.openjdk.org/browse/JDK-8315129 > > It is mostly straightforward - the module imports are parsed, and then > expanded to import-on-demand in `TypeEnter`. > There is a few notable aspects, however: > - the AST node for import (`JCImport`) is holding the imported element as a > field access, because so far, the imported element always had to have a '.' > (even for import-on-demand). But for module imports, it is permissible to > import from a module whose name does not have a dot (`import module m;`). The > use of field access for ordinary import seems very useful, so I preferred to > keep that, and created a new internal-only AST node for module imports. There > is still only one public API AST node/interface, so this is purely an > implementation choice. > - JShell now supports module imports as well; and the default, implicit, > script is changed to use it to import all of `java.base` if preview is > enabled. It is expected that the default would be changed if/when the module > imports feature is finalized. Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 20 commits: - Merge branch 'master' into module-imports - Including current module name as suggested. - Adding more tests for ambiguities. - Merge branch 'master' into module-imports - Merge branch 'master' into module-imports - Merge branch 'master' into module-imports - Fixing test. - Fixing disambiguation of module imports. - Fixing file name computation. - Merge branch 'master' into module-imports - ... and 10 more: https://git.openjdk.org/jdk/compare/03e84178...c75b8b7a - Changes: https://git.openjdk.org/jdk/pull/18614/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18614=05 Stats: 1198 lines in 29 files changed: 1130 ins; 9 del; 59 mod Patch: https://git.openjdk.org/jdk/pull/18614.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18614/head:pull/18614 PR: https://git.openjdk.org/jdk/pull/18614
Re: RFR: 8329420: Java 22 (and 23) launcher calls default constructor although main() is static [v4]
> Consider code like:
>
> public class MainClass {
> public MainClass() {
> System.out.println("Constructor called!");
> }
> public static void main() {
> System.out.println("main called!");
> }
> }
>
> and compile and run it, with preview enabled, like:
>
> $ javac /tmp/MainClass.java
> $ java --enable-preview -classpath /tmp MainClass
> Constructor called!
> main called!
>
>
> That is wrong, as the `main` method is static, and there is no need to create
> a new instance of the class.
>
> The reason is that as launcher attempts to invoke the main method, it goes in
> the following order: 1) static-main-with-args; 2) instance-main-with-args; 3)
> static-main-without-args; 4) instance-main-without-args. But, for the
> instance variants, the code first creates a new instance of the given class,
> and only then attempts to lookup the `main` method, and will pass to the next
> option when the `main` method lookup fails. So, when invoking
> static-main-without-args, the current class instance may be created for
> instance-main-with-args, which will then fail due to the missing
> `main(String[])` method.
>
> The proposed solution to this problem is to simply first do a lookup for the
> `main` method (skipping to the next variant when the given main method does
> not exist, without instantiating the current class).
>
> There is also a relatively closely related problem: what happens when the
> constructor throws an exception?
>
> public class MainClass {
> public MainClass() {
> if (true) throw new RuntimeException();
> }
> public void main() {
> System.out.println("main called!");
> }
> }
>
>
> when compiled an run, this produces no output whatsoever:
>
> $ javac /tmp/MainClass.java
> $ java --enable-preview -classpath /tmp MainClass
> $
>
>
> This is because any exceptions thrown from the constructor are effectively
> ignored, and the launcher will continue with the next variant. This seems
> wrong - the exception should be printed for the user, like:
>
> $ java --enable-preview -classpath /tmp/ MainClass
> Exception in thread "main" java.lang.RuntimeException
> at MainClass.(MainClass.java:3)
>
>
> This patch proposes to do that by not consuming the exceptions thrown from
> the constructor, and stop the propagation to the next variant.
Jan Lahoda has updated the pull request incrementally with one additional
commit since the last revision:
Fixing typo (should not continue after an exception from a constructor).
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18753/files
- new: https://git.openjdk.org/jdk/pull/18753/files/c007f61e..aadfb380
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18753=03
- incr: https://webrevs.openjdk.org/?repo=jdk=18753=02-03
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/18753.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18753/head:pull/18753
PR: https://git.openjdk.org/jdk/pull/18753
Re: RFR: 8329420: Java 22 (and 23) launcher calls default constructor although main() is static [v3]
On Wed, 17 Apr 2024 08:51:39 GMT, Jaikiran Pai wrote: >> Jan Lahoda has updated the pull request incrementally with three additional >> commits since the last revision: >> >> - Reflecting code formatting suggestion. >> - First lookup the main method, and only then the constructor. >> - Attempting to solve JDK-8329581 by only ignoring j.l.NoSuchMethodError > > src/java.base/share/native/libjli/java.c line 477: > >> 475: // and don't continue with the next variant; >> 476: // leave any exception pending, so that it is visible to the >> caller: >> 477: return 1; > > Hello Jan, the comment says "don't continue", yet this returns `1` which > represents "continue". Should this return `0` instead? Uh, you are right of course. Thanks for noticing! Fixed. (I believe the difference here is not currently observable, as this is the last step in the cascade, but may become observable if the order changes.) - PR Review Comment: https://git.openjdk.org/jdk/pull/18753#discussion_r1568511740
Re: RFR: 8329420: Java 22 (and 23) launcher calls default constructor although main() is static [v3]
On Wed, 17 Apr 2024 06:34:25 GMT, Jan Lahoda wrote:
>> Consider code like:
>>
>> public class MainClass {
>> public MainClass() {
>> System.out.println("Constructor called!");
>> }
>> public static void main() {
>> System.out.println("main called!");
>> }
>> }
>>
>> and compile and run it, with preview enabled, like:
>>
>> $ javac /tmp/MainClass.java
>> $ java --enable-preview -classpath /tmp MainClass
>> Constructor called!
>> main called!
>>
>>
>> That is wrong, as the `main` method is static, and there is no need to
>> create a new instance of the class.
>>
>> The reason is that as launcher attempts to invoke the main method, it goes
>> in the following order: 1) static-main-with-args; 2)
>> instance-main-with-args; 3) static-main-without-args; 4)
>> instance-main-without-args. But, for the instance variants, the code first
>> creates a new instance of the given class, and only then attempts to lookup
>> the `main` method, and will pass to the next option when the `main` method
>> lookup fails. So, when invoking static-main-without-args, the current class
>> instance may be created for instance-main-with-args, which will then fail
>> due to the missing `main(String[])` method.
>>
>> The proposed solution to this problem is to simply first do a lookup for the
>> `main` method (skipping to the next variant when the given main method does
>> not exist, without instantiating the current class).
>>
>> There is also a relatively closely related problem: what happens when the
>> constructor throws an exception?
>>
>> public class MainClass {
>> public MainClass() {
>> if (true) throw new RuntimeException();
>> }
>> public void main() {
>> System.out.println("main called!");
>> }
>> }
>>
>>
>> when compiled an run, this produces no output whatsoever:
>>
>> $ javac /tmp/MainClass.java
>> $ java --enable-preview -classpath /tmp MainClass
>> $
>>
>>
>> This is because any exceptions thrown from the constructor are effectively
>> ignored, and the launcher will continue with the next variant. This seems
>> wrong - the exception should be printed for the user, like:
>>
>> $ java --enable-preview -classpath /tmp/ MainClass
>> Exception in thread "main" java.lang.RuntimeException
>> at MainClass.(MainClass.java:3)
>>
>>
>> This patch proposes to do that by not consuming the exceptions thrown from
>> the constructor, and stop the propagation to the next variant.
>
> Jan Lahoda has updated the pull request incrementally with three additional
> commits since the last revision:
>
> - Reflecting code formatting suggestion.
> - First lookup the main method, and only then the constructor.
> - Attempting to solve JDK-8329581 by only ignoring j.l.NoSuchMethodError
So, I've pushed an update that:
- attemps to solve [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581)
by only ignoring `NoSuchMethodError` when looking up methods. Hence any other
problem will lead to an error to be printed out, as usually.
https://github.com/openjdk/jdk/pull/18753/commits/dd447e898785a3d7e2def47dd1efcca3db41dd46
- changed the code to first lookup the `main` method, and only then
constructors, which is what I think @dholmes-ora was suggesting.
https://github.com/openjdk/jdk/pull/18753/commits/83fabec6e5e8bf6deb9efce22ef6ace408cf3d26
- fixed the comment formatting, as suggested by @AlanBateman
https://github.com/openjdk/jdk/pull/18753/commits/c007f61ebc0660cc6f93e29e71dc635aa50ba538
Please let me know what you think. Thanks!
-
PR Comment: https://git.openjdk.org/jdk/pull/18753#issuecomment-2060481245
Re: RFR: 8329420: Java 22 (and 23) launcher calls default constructor although main() is static [v3]
> Consider code like:
>
> public class MainClass {
> public MainClass() {
> System.out.println("Constructor called!");
> }
> public static void main() {
> System.out.println("main called!");
> }
> }
>
> and compile and run it, with preview enabled, like:
>
> $ javac /tmp/MainClass.java
> $ java --enable-preview -classpath /tmp MainClass
> Constructor called!
> main called!
>
>
> That is wrong, as the `main` method is static, and there is no need to create
> a new instance of the class.
>
> The reason is that as launcher attempts to invoke the main method, it goes in
> the following order: 1) static-main-with-args; 2) instance-main-with-args; 3)
> static-main-without-args; 4) instance-main-without-args. But, for the
> instance variants, the code first creates a new instance of the given class,
> and only then attempts to lookup the `main` method, and will pass to the next
> option when the `main` method lookup fails. So, when invoking
> static-main-without-args, the current class instance may be created for
> instance-main-with-args, which will then fail due to the missing
> `main(String[])` method.
>
> The proposed solution to this problem is to simply first do a lookup for the
> `main` method (skipping to the next variant when the given main method does
> not exist, without instantiating the current class).
>
> There is also a relatively closely related problem: what happens when the
> constructor throws an exception?
>
> public class MainClass {
> public MainClass() {
> if (true) throw new RuntimeException();
> }
> public void main() {
> System.out.println("main called!");
> }
> }
>
>
> when compiled an run, this produces no output whatsoever:
>
> $ javac /tmp/MainClass.java
> $ java --enable-preview -classpath /tmp MainClass
> $
>
>
> This is because any exceptions thrown from the constructor are effectively
> ignored, and the launcher will continue with the next variant. This seems
> wrong - the exception should be printed for the user, like:
>
> $ java --enable-preview -classpath /tmp/ MainClass
> Exception in thread "main" java.lang.RuntimeException
> at MainClass.(MainClass.java:3)
>
>
> This patch proposes to do that by not consuming the exceptions thrown from
> the constructor, and stop the propagation to the next variant.
Jan Lahoda has updated the pull request incrementally with three additional
commits since the last revision:
- Reflecting code formatting suggestion.
- First lookup the main method, and only then the constructor.
- Attempting to solve JDK-8329581 by only ignoring j.l.NoSuchMethodError
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18753/files
- new: https://git.openjdk.org/jdk/pull/18753/files/2022aa5a..c007f61e
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18753=02
- incr: https://webrevs.openjdk.org/?repo=jdk=18753=01-02
Stats: 117 lines in 2 files changed: 73 ins; 5 del; 39 mod
Patch: https://git.openjdk.org/jdk/pull/18753.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18753/head:pull/18753
PR: https://git.openjdk.org/jdk/pull/18753
Re: RFR: 8329420: Java 22 (and 23) launcher calls default constructor although main() is static [v2]
On Tue, 16 Apr 2024 10:03:21 GMT, David Holmes wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Reflecting review feedback.
>
> src/java.base/share/native/libjli/java.c line 419:
>
>> 417: invokeInstanceMainWithArgs(JNIEnv *env, jclass mainClass, jobjectArray
>> mainArgs) {
>> 418: jmethodID constructor = (*env)->GetMethodID(env, mainClass,
>> "", "()V");
>> 419: CHECK_EXCEPTION_FAIL();
>
> Shouldn't this also not be checked until you know you have an instance method?
You mean to first check whether the method exists, and only then try to lookup
the constructor? Not sure if that has observable effects, but I can do that.
I missed [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581) before, my
plan is to adjust this "check for exception and clear it" check to only work
for `NoSuchMethodError`, and let the exception pass for any other exception.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18753#discussion_r1567361164
Re: RFR: 8329420: Java 22 (and 23) launcher calls default constructor although main() is static [v2]
> Consider code like:
>
> public class MainClass {
> public MainClass() {
> System.out.println("Constructor called!");
> }
> public static void main() {
> System.out.println("main called!");
> }
> }
>
> and compile and run it, with preview enabled, like:
>
> $ javac /tmp/MainClass.java
> $ java --enable-preview -classpath /tmp MainClass
> Constructor called!
> main called!
>
>
> That is wrong, as the `main` method is static, and there is no need to create
> a new instance of the class.
>
> The reason is that as launcher attempts to invoke the main method, it goes in
> the following order: 1) static-main-with-args; 2) instance-main-with-args; 3)
> static-main-without-args; 4) instance-main-without-args. But, for the
> instance variants, the code first creates a new instance of the given class,
> and only then attempts to lookup the `main` method, and will pass to the next
> option when the `main` method lookup fails. So, when invoking
> static-main-without-args, the current class instance may be created for
> instance-main-with-args, which will then fail due to the missing
> `main(String[])` method.
>
> The proposed solution to this problem is to simply first do a lookup for the
> `main` method (skipping to the next variant when the given main method does
> not exist, without instantiating the current class).
>
> There is also a relatively closely related problem: what happens when the
> constructor throws an exception?
>
> public class MainClass {
> public MainClass() {
> if (true) throw new RuntimeException();
> }
> public void main() {
> System.out.println("main called!");
> }
> }
>
>
> when compiled an run, this produces no output whatsoever:
>
> $ javac /tmp/MainClass.java
> $ java --enable-preview -classpath /tmp MainClass
> $
>
>
> This is because any exceptions thrown from the constructor are effectively
> ignored, and the launcher will continue with the next variant. This seems
> wrong - the exception should be printed for the user, like:
>
> $ java --enable-preview -classpath /tmp/ MainClass
> Exception in thread "main" java.lang.RuntimeException
> at MainClass.(MainClass.java:3)
>
>
> This patch proposes to do that by not consuming the exceptions thrown from
> the constructor, and stop the propagation to the next variant.
Jan Lahoda has updated the pull request incrementally with one additional
commit since the last revision:
Reflecting review feedback.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18753/files
- new: https://git.openjdk.org/jdk/pull/18753/files/3ad521ae..2022aa5a
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18753=01
- incr: https://webrevs.openjdk.org/?repo=jdk=18753=00-01
Stats: 22 lines in 1 file changed: 10 ins; 10 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/18753.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18753/head:pull/18753
PR: https://git.openjdk.org/jdk/pull/18753
Re: RFR: 8329420: Java 22 (and 23) launcher calls default constructor although main() is static [v2]
On Sun, 14 Apr 2024 06:51:37 GMT, Alan Bateman wrote: >> Jan Lahoda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Reflecting review feedback. > > src/java.base/share/native/libjli/java.c line 434: > >> 432: CHECK_EXCEPTION_FAIL(); >> 433: jobject mainObject = (*env)->NewObject(env, mainClass, constructor); >> 434: CHECK_EXCEPTION_NULL_PASS(mainObject); > > There will be a pending exception if NewObject returns NULL. I think it would > be a lot easier to read if this were to just check if mainObject is NULL and > return 1. Same comment for the other usage in invokeInstanceMainWithoutArgs. > That would avoid CHECK_EXCEPTION_NULL_PASS which is very confusing to see at > the use-sites. Thanks! I've done that here: https://github.com/openjdk/jdk/pull/18753/commits/2022aa5a0d21f930d9b49d1bb0b91ecf7e60ead2 - PR Review Comment: https://git.openjdk.org/jdk/pull/18753#discussion_r1565302792
RFR: 8329420: Java 22 (and 23) launcher calls default constructor although main() is static
Consider code like:
public class MainClass {
public MainClass() {
System.out.println("Constructor called!");
}
public static void main() {
System.out.println("main called!");
}
}
and compile and run it, with preview enabled, like:
$ javac /tmp/MainClass.java
$ java --enable-preview -classpath /tmp MainClass
Constructor called!
main called!
That is wrong, as the `main` method is static, and there is no need to create a
new instance of the class.
The reason is that as launcher attempts to invoke the main method, it goes in
the following order: 1) static-main-with-args; 2) instance-main-with-args; 3)
static-main-without-args; 4) instance-main-without-args. But, for the instance
variants, the code first creates a new instance of the given class, and only
then attempts to lookup the `main` method, and will pass to the next option
when the `main` method lookup fails. So, when invoking
static-main-without-args, the current class instance may be created for
instance-main-with-args, which will then fail due to the missing
`main(String[])` method.
The proposed solution to this problem is to simply first do a lookup for the
`main` method (skipping to the next variant when the given main method does not
exist, without instantiating the current class).
There is also a relatively closely related problem: what happens when the
constructor throws an exception?
public class MainClass {
public MainClass() {
if (true) throw new RuntimeException();
}
public void main() {
System.out.println("main called!");
}
}
when compiled an run, this produces no output whatsoever:
$ javac /tmp/MainClass.java
$ java --enable-preview -classpath /tmp MainClass
$
This is because any exceptions thrown from the constructor are effectively
ignored, and the launcher will continue with the next variant. This seems wrong
- the exception should be printed for the user, like:
$ java --enable-preview -classpath /tmp/ MainClass
Exception in thread "main" java.lang.RuntimeException
at MainClass.(MainClass.java:3)
This patch proposes to do that by not consuming the exceptions thrown from the
constructor, and stop the propagation to the next variant.
-
Commit messages:
- Updating year.
- 8329420: Java 22 (and 23) launcher calls default constructor although main()
is static
Changes: https://git.openjdk.org/jdk/pull/18753/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=18753=00
Issue: https://bugs.openjdk.org/browse/JDK-8329420
Stats: 150 lines in 2 files changed: 130 ins; 4 del; 16 mod
Patch: https://git.openjdk.org/jdk/pull/18753.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18753/head:pull/18753
PR: https://git.openjdk.org/jdk/pull/18753
Re: RFR: 8329948: Remove string template feature [v4]
On Wed, 10 Apr 2024 10:59:25 GMT, Maurizio Cimadamore
wrote:
>> This PR removes support for the string template feature from the Java
>> compiler and the Java SE API, as discussed here:
>>
>> https://mail.openjdk.org/pipermail/amber-spec-experts/2024-April/004106.html
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix issues in digit classes
javac and JShell changes look good to me (with a nit in JShell tests).
For consideration: using `{` will now produce the "illegal escape character"
error. Which is technically correct, but maybe we could add a special error,
saying that StringTemplates are removed for now? So that if someone will try to
compile source code with StringTemplates, they would now this was intentional.
Just for consideration.
test/langtools/jdk/jshell/CompletionSuggestionTest.java line 327:
> 325: assertSignature("String.format(|",
> 326: "String String.format(String, Object...)",
> 327: "String String.format(java.util.Locale, String,
> Object...)"
Nit: this change appears to be unnecessary?
-
Marked as reviewed by jlahoda (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18688#pullrequestreview-1993673798
PR Review Comment: https://git.openjdk.org/jdk/pull/18688#discussion_r1560692978
Re: RFR: 8329948: Remove string template feature [v4]
On Wed, 10 Apr 2024 10:44:13 GMT, Maurizio Cimadamore wrote: >> src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line >> 120: >> >>> 118: * @since 21 >>> 119: */ >>> 120: public static final int MAX_INDY_CONCAT_ARG_SLOTS; >> >> May this value change in the future? If yes, we might change this to a >> method to avoid incorrect eager inlining by the java compiler, or drop this >> with string templates. > > Good catch. Since this was tweaked to be a public API as part of the string > template feature, I've reverted this code to what it was prior to string > template. That is, now this is a private static final constant, with > initializer set to 200 (no need to inhibit javac constant folding, since all > uses are from this file). Just for completeness - note the field is not initialized in its initializator, but using a static init below. Fields like this are not inlined as constants by javac - that would require the value to be provided in the field initializator. - PR Review Comment: https://git.openjdk.org/jdk/pull/18688#discussion_r1559643723
Re: RFR: 8328481: Implement Module Imports [v4]
On Wed, 10 Apr 2024 08:41:19 GMT, Christian Stein wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Adding more tests for ambiguities.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties
> line 3553:
>
>> 3551: # 0: symbol
>> 3552: compiler.err.import.module.does.not.read=\
>> 3553: current modules does not read: {0}
>
> Suggestion:
>
> current module does not read: {0}
>
>
> It would even be better to print the name of the "current" module, so that
> the user-facing message reads:
> `current module foo does not read module bar - possible fix: add "requires
> bar;" to foo's module descriptor `
Thanks. I've tweaked the error to include the module name here:
https://github.com/openjdk/jdk/pull/18614/commits/7cfaff80eac6261b62c5cdb9614a0e708bce7e33
Regarding possible fix(es) - it may be better to introduce some framework for
that, and convert the most typical errors, separately. (As I don't think we do
that for other errors.)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18614#discussion_r1559616761
Re: RFR: 8328481: Implement Module Imports [v5]
> This is an implementation of JEP JDK-8315129: Module Import Declarations > (Preview). Please see the JEP for details: > https://bugs.openjdk.org/browse/JDK-8315129 > > It is mostly straightforward - the module imports are parsed, and then > expanded to import-on-demand in `TypeEnter`. > There is a few notable aspects, however: > - the AST node for import (`JCImport`) is holding the imported element as a > field access, because so far, the imported element always had to have a '.' > (even for import-on-demand). But for module imports, it is permissible to > import from a module whose name does not have a dot (`import module m;`). The > use of field access for ordinary import seems very useful, so I preferred to > keep that, and created a new internal-only AST node for module imports. There > is still only one public API AST node/interface, so this is purely an > implementation choice. > - JShell now supports module imports as well; and the default, implicit, > script is changed to use it to import all of `java.base` if preview is > enabled. It is expected that the default would be changed if/when the module > imports feature is finalized. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Including current module name as suggested. - Changes: - all: https://git.openjdk.org/jdk/pull/18614/files - new: https://git.openjdk.org/jdk/pull/18614/files/0ca05b7d..7cfaff80 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18614=04 - incr: https://webrevs.openjdk.org/?repo=jdk=18614=03-04 Stats: 76 lines in 4 files changed: 73 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18614.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18614/head:pull/18614 PR: https://git.openjdk.org/jdk/pull/18614
Re: RFR: 8328481: Implement Module Imports [v4]
On Wed, 10 Apr 2024 11:52:31 GMT, Maurizio Cimadamore
wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Adding more tests for ambiguities.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/tree/JCTree.java line 719:
>
>> 717: public static class JCModuleImport extends JCImportBase {
>> 718: /** The module name. */
>> 719: public JCExpression module;
>
> Does it need to be an expression? Or is a name enough?
Expression is more consistent with e.g. the module directives (`JCRequires` has
`JCExpression moduleName`). Also allows us to keep positions for the name.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18614#discussion_r1559440577
Re: RFR: 8328481: Implement Module Imports [v4]
> This is an implementation of JEP JDK-8315129: Module Import Declarations > (Preview). Please see the JEP for details: > https://bugs.openjdk.org/browse/JDK-8315129 > > It is mostly straightforward - the module imports are parsed, and then > expanded to import-on-demand in `TypeEnter`. > There is a few notable aspects, however: > - the AST node for import (`JCImport`) is holding the imported element as a > field access, because so far, the imported element always had to have a '.' > (even for import-on-demand). But for module imports, it is permissible to > import from a module whose name does not have a dot (`import module m;`). The > use of field access for ordinary import seems very useful, so I preferred to > keep that, and created a new internal-only AST node for module imports. There > is still only one public API AST node/interface, so this is purely an > implementation choice. > - JShell now supports module imports as well; and the default, implicit, > script is changed to use it to import all of `java.base` if preview is > enabled. It is expected that the default would be changed if/when the module > imports feature is finalized. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Adding more tests for ambiguities. - Changes: - all: https://git.openjdk.org/jdk/pull/18614/files - new: https://git.openjdk.org/jdk/pull/18614/files/c44031ed..0ca05b7d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18614=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18614=02-03 Stats: 69 lines in 1 file changed: 68 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18614.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18614/head:pull/18614 PR: https://git.openjdk.org/jdk/pull/18614
Re: RFR: 8328481: Implement Module Imports [v3]
> This is an implementation of JEP JDK-8315129: Module Import Declarations > (Preview). Please see the JEP for details: > https://bugs.openjdk.org/browse/JDK-8315129 > > It is mostly straightforward - the module imports are parsed, and then > expanded to import-on-demand in `TypeEnter`. > There is a few notable aspects, however: > - the AST node for import (`JCImport`) is holding the imported element as a > field access, because so far, the imported element always had to have a '.' > (even for import-on-demand). But for module imports, it is permissible to > import from a module whose name does not have a dot (`import module m;`). The > use of field access for ordinary import seems very useful, so I preferred to > keep that, and created a new internal-only AST node for module imports. There > is still only one public API AST node/interface, so this is purely an > implementation choice. > - JShell now supports module imports as well; and the default, implicit, > script is changed to use it to import all of `java.base` if preview is > enabled. It is expected that the default would be changed if/when the module > imports feature is finalized. Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 17 commits: - Merge branch 'master' into module-imports - Merge branch 'master' into module-imports - Merge branch 'master' into module-imports - Fixing test. - Fixing disambiguation of module imports. - Fixing file name computation. - Merge branch 'master' into module-imports - Cleanup. - Fixing CheckExamples. - Adding support for import modules to JShell. - ... and 7 more: https://git.openjdk.org/jdk/compare/b9331cd2...c44031ed - Changes: https://git.openjdk.org/jdk/pull/18614/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18614=02 Stats: 1065 lines in 28 files changed: 989 ins; 11 del; 65 mod Patch: https://git.openjdk.org/jdk/pull/18614.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18614/head:pull/18614 PR: https://git.openjdk.org/jdk/pull/18614
Re: RFR: 8324651: Compiler Implementation for Derived Record Creation (Preview) [v9]
On Mon, 8 Apr 2024 16:36:49 GMT, Vicente Romero wrote: >> Jan Lahoda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Adding tests as suggested. > > src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java > line 101: > >> 99: import com.sun.tools.javac.tree.JCTree.JCPattern; >> 100: import com.sun.tools.javac.tree.JCTree.JCPatternCaseLabel; >> 101: import com.sun.tools.javac.tree.JCTree.JCDerivedInstance; > > changes to this file can be removed now Thanks, removed: https://github.com/openjdk/jdk/pull/18509/commits/a05480cd1a436014ba00505db71d54d82a37ecd1 > test/langtools/tools/javac/patterns/withers/Model.java line 2: > >> 1: /* >> 2: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. > > probably this test should be in another location not under `patterns`, same > for other tests added inside `patterns` folder Thanks, moved: https://github.com/openjdk/jdk/pull/18509/commits/a05480cd1a436014ba00505db71d54d82a37ecd1 - PR Review Comment: https://git.openjdk.org/jdk/pull/18509#discussion_r1557304771 PR Review Comment: https://git.openjdk.org/jdk/pull/18509#discussion_r1557304921
Re: RFR: 8324651: Compiler Implementation for Derived Record Creation (Preview) [v10]
> This is a patch for javac, that adds the Derived Record Creation expressions. > The current draft specification for the feature is: > https://cr.openjdk.org/~gbierman/jep468/jep468-20240326/specs/derived-record-creation-jls.html > > The current CSR is here: > https://bugs.openjdk.org/browse/JDK-8328637 > > The patch is mostly straightforward, with two notable changes: > - there is a new `ElementKind.COMPONENT_LOCAL_VARIABLE`, as the > specification introduces this term, and it seems consistent with > `ElementKind.BINDING_VARIABLE` that was introduced some time ago. > - there are a bit broader changes in `Flow`, to facilitate the introduction > of variables without an explicit declaration for definite assignment and > effectively final computation. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Reflecting review feedback: - reverting unnecessary changes in TransPatterns - moving the patters/withers/Model test to a more appropriate place - Changes: - all: https://git.openjdk.org/jdk/pull/18509/files - new: https://git.openjdk.org/jdk/pull/18509/files/e0930688..a05480cd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18509=09 - incr: https://webrevs.openjdk.org/?repo=jdk=18509=08-09 Stats: 37 lines in 4 files changed: 14 ins; 17 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/18509.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18509/head:pull/18509 PR: https://git.openjdk.org/jdk/pull/18509
Re: RFR: 8325485: IncrementInstructions.of(int, int) is not storing the args [v2]
On Fri, 9 Feb 2024 10:47:08 GMT, Adam Sotona wrote: >> ClassFile API provides two sets of instructions implementations (bound and >> unbound). >> Unbound implementation of `IncrementInstruction::constant` returns invalid >> value. >> This bug discovered a hole in the ClassFile API test coverage. >> >> This patch provides very simple fix of `IncrementInstruction` >> and more complex fix of the test framework to cover all unbound instruction >> with automated tests. >> >> The test framework fix includes correction of hash calculation of >> instructions in `ClassRecord` and >> two-pass transformation in `RebuildingTransformation`. Second pass has been >> added to discover bugs in unbound-to-unbound instruction conversions. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > Update test/jdk/jdk/classfile/helpers/RebuildingTransformation.java > > Co-authored-by: Andrey Turbanov Looks reasonable. - Marked as reviewed by jlahoda (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17770#pullrequestreview-1988500946
Re: RFR: 8329527: Opcode.IFNONNULL.primaryTypeKind() is not ReferenceType
On Wed, 3 Apr 2024 08:15:52 GMT, Adam Sotona wrote: > `Opcode.IFNONNULL.primaryTypeKind()` wrongly returned `IntType` instead of > the right `ReferenceType`. > Primary type kinds of `BRANCH` opcodes have yet no functional effect in the > Class-File API. > This simple patch fixes the typo. > > Please review. > > Thank you, > Adam Looks good. - Marked as reviewed by jlahoda (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18593#pullrequestreview-1988500479
Re: RFR: 8324651: Compiler Implementation for Derived Record Creation (Preview) [v8]
On Fri, 5 Apr 2024 18:30:30 GMT, Joe Darcy wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Reflecting review feedback:
>> - pre-generating the JCVarDecls in Attr, to aid Flow
>> - adding a note on how the desugared code looks like
>
> src/java.compiler/share/classes/javax/lang/model/element/ElementKind.java
> line 135:
>
>> 133: COMPONENT_LOCAL_VARIABLE;
>> 134:
>> 135: // Maintenance note: check if the default implementation of
>
> From a quick look, I don't think Elements.getOutermostTypeElement needs
> updating for COMPONENT_LOCAL_VARIABLE, but it would be good to add a test
> case.
Tests added:
https://github.com/openjdk/jdk/pull/18509/commits/e09306885e5c6cd2aba7779025fc7efcb8991e8e
Thanks!
> src/java.compiler/share/classes/javax/lang/model/util/ElementKindVisitorPreview.java
> line 91:
>
>> 89:
>> 90: /**
>> 91: * {@inheritDoc ElementKindVisitor6}
>
> If possible, would be good to add some minimal testing/use of the element
> kind visitors on component local variables.
Tests added:
https://github.com/openjdk/jdk/pull/18509/commits/e09306885e5c6cd2aba7779025fc7efcb8991e8e
Thanks!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18509#discussion_r1557243414
PR Review Comment: https://git.openjdk.org/jdk/pull/18509#discussion_r1557243525
Re: RFR: 8324651: Compiler Implementation for Derived Record Creation (Preview) [v9]
> This is a patch for javac, that adds the Derived Record Creation expressions. > The current draft specification for the feature is: > https://cr.openjdk.org/~gbierman/jep468/jep468-20240326/specs/derived-record-creation-jls.html > > The current CSR is here: > https://bugs.openjdk.org/browse/JDK-8328637 > > The patch is mostly straightforward, with two notable changes: > - there is a new `ElementKind.COMPONENT_LOCAL_VARIABLE`, as the > specification introduces this term, and it seems consistent with > `ElementKind.BINDING_VARIABLE` that was introduced some time ago. > - there are a bit broader changes in `Flow`, to facilitate the introduction > of variables without an explicit declaration for definite assignment and > effectively final computation. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Adding tests as suggested. - Changes: - all: https://git.openjdk.org/jdk/pull/18509/files - new: https://git.openjdk.org/jdk/pull/18509/files/14651358..e0930688 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18509=08 - incr: https://webrevs.openjdk.org/?repo=jdk=18509=07-08 Stats: 261 lines in 3 files changed: 257 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/18509.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18509/head:pull/18509 PR: https://git.openjdk.org/jdk/pull/18509
Re: RFR: 8324651: Compiler Implementation for Derived Record Creation (Preview) [v7]
On Fri, 5 Apr 2024 13:09:56 GMT, Maurizio Cimadamore wrote: >> Jan Lahoda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> JavaCompiler cleanup > > src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 2117: > >> 2115: * position. >> 2116: */ >> 2117: protected VarAndDeclarationTree[] vars; > > So... normally, a `JCVariableDecl` would have a symbol attached, which is how > the old code used to work. Is it correct that this change is needed because > the `componentLocalVariables` list is a list of symbols and not of > JCVariableDecl? Would it be too complex to create a declaration in Attr and > save those in the derived creation tree? Generating the `JCVariableDecl`s in Attr in https://github.com/openjdk/jdk/pull/18509/commits/146513580f96abd5feb7886bd1191805cc93403b. Thanks for the comment! > src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java line 4443: > >> 4441: } >> 4442: >> 4443: @Override > > As usual, I suggest to add some brief comment with the shape of the generated > code. Done here: https://github.com/openjdk/jdk/pull/18509/commits/146513580f96abd5feb7886bd1191805cc93403b#diff-bc0df6dce7f74078bfca1e90bec75d7bb3d8b338933be725da78f0a8a7a0c78dR4445 Please let me know if that needs some tweaks. Thanks for the comment! - PR Review Comment: https://git.openjdk.org/jdk/pull/18509#discussion_r1554103472 PR Review Comment: https://git.openjdk.org/jdk/pull/18509#discussion_r1554104437
Re: RFR: 8324651: Compiler Implementation for Derived Record Creation (Preview) [v8]
> This is a patch for javac, that adds the Derived Record Creation expressions. > The current draft specification for the feature is: > https://cr.openjdk.org/~gbierman/jep468/jep468-20240326/specs/derived-record-creation-jls.html > > The current CSR is here: > https://bugs.openjdk.org/browse/JDK-8328637 > > The patch is mostly straightforward, with two notable changes: > - there is a new `ElementKind.COMPONENT_LOCAL_VARIABLE`, as the > specification introduces this term, and it seems consistent with > `ElementKind.BINDING_VARIABLE` that was introduced some time ago. > - there are a bit broader changes in `Flow`, to facilitate the introduction > of variables without an explicit declaration for definite assignment and > effectively final computation. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Reflecting review feedback: - pre-generating the JCVarDecls in Attr, to aid Flow - adding a note on how the desugared code looks like - Changes: - all: https://git.openjdk.org/jdk/pull/18509/files - new: https://git.openjdk.org/jdk/pull/18509/files/c91e87fd..14651358 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18509=07 - incr: https://webrevs.openjdk.org/?repo=jdk=18509=06-07 Stats: 75 lines in 6 files changed: 30 ins; 11 del; 34 mod Patch: https://git.openjdk.org/jdk/pull/18509.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18509/head:pull/18509 PR: https://git.openjdk.org/jdk/pull/18509
Re: RFR: 8324651: Compiler Implementation for Derived Record Creation (Preview) [v7]
> This is a patch for javac, that adds the Derived Record Creation expressions. > The current draft specification for the feature is: > https://cr.openjdk.org/~gbierman/jep468/jep468-20240326/specs/derived-record-creation-jls.html > > The current CSR is here: > https://bugs.openjdk.org/browse/JDK-8328637 > > The patch is mostly straightforward, with two notable changes: > - there is a new `ElementKind.COMPONENT_LOCAL_VARIABLE`, as the > specification introduces this term, and it seems consistent with > `ElementKind.BINDING_VARIABLE` that was introduced some time ago. > - there are a bit broader changes in `Flow`, to facilitate the introduction > of variables without an explicit declaration for definite assignment and > effectively final computation. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: JavaCompiler cleanup - Changes: - all: https://git.openjdk.org/jdk/pull/18509/files - new: https://git.openjdk.org/jdk/pull/18509/files/f9b6c403..c91e87fd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18509=06 - incr: https://webrevs.openjdk.org/?repo=jdk=18509=05-06 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18509.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18509/head:pull/18509 PR: https://git.openjdk.org/jdk/pull/18509
Re: RFR: 8324651: Compiler Implementation for Derived Record Creation (Preview) [v5]
On Thu, 4 Apr 2024 17:00:33 GMT, Vicente Romero wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixing tests.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/main/JavaCompiler.java
> line 1574:
>
>> 1572: @Override
>> 1573: public void visitDerivedInstance(JCDerivedInstance tree) {
>> 1574: hasPatterns |= true;
>
> it could be thought as a pattern but it is not a pattern nor it is internally
> converted to a pattern, unless I'm missing something
The main point here is to make not run `TransPatterns` in case the visitor is
not performing any work (because loading the class causes some perf tests to
regress - we do the same for `LambdaToMethod`). But, as the desugaring was
moved to `Lower`, we no longer need to do anything in `JavaCompiler`. Thanks
for the comment!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18509#discussion_r1553555861
Re: RFR: 8324651: Compiler Implementation for Derived Record Creation (Preview) [v6]
> This is a patch for javac, that adds the Derived Record Creation expressions. > The current draft specification for the feature is: > https://cr.openjdk.org/~gbierman/jep468/jep468-20240326/specs/derived-record-creation-jls.html > > The current CSR is here: > https://bugs.openjdk.org/browse/JDK-8328637 > > The patch is mostly straightforward, with two notable changes: > - there is a new `ElementKind.COMPONENT_LOCAL_VARIABLE`, as the > specification introduces this term, and it seems consistent with > `ElementKind.BINDING_VARIABLE` that was introduced some time ago. > - there are a bit broader changes in `Flow`, to facilitate the introduction > of variables without an explicit declaration for definite assignment and > effectively final computation. Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 29 commits: - Improving the javax.lang.model for component local variables, by darcy. - Reflecting review feedback. - Merge branch 'master' into wthexp - Fixing tests. - Adding tests. - The var declaration can be any JCTree. - Renaming visitReconstruction to visitDerivedInstance as suggested. - Fixing support for derived record creation expression in JShell. - Removing whitespace - Cleanup. - ... and 19 more: https://git.openjdk.org/jdk/compare/18c925cd...f9b6c403 - Changes: https://git.openjdk.org/jdk/pull/18509/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18509=05 Stats: 1772 lines in 50 files changed: 1687 ins; 20 del; 65 mod Patch: https://git.openjdk.org/jdk/pull/18509.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18509/head:pull/18509 PR: https://git.openjdk.org/jdk/pull/18509
Re: RFR: 8324651: Compiler Implementation for Derived Record Creation (Preview) [v5]
On Thu, 4 Apr 2024 16:47:07 GMT, Vicente Romero wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixing tests.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 2235:
>
>> 2233: }
>> 2234: sym.adr = nextadr;
>> 2235: vars[nextadr] = sym;
>
> could we use a record here? these two arrays seem to be in sync
Sure, done. Thanks for the comment!
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java
> line 1364:
>
>> 1362:
>> 1363: @Override
>> 1364: public void visitDerivedInstance(JCDerivedInstance tree) {
>
> I was expecting this code to belong to Lower, not saying that it is wrong
> here though but probably more naturally in Lower I think
Moved to `Lower`. Thanks for the comment!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18509#discussion_r1553545686
PR Review Comment: https://git.openjdk.org/jdk/pull/18509#discussion_r1553546716
Re: RFR: 8324651: Compiler Implementation for Derived Record Creation (Preview) [v5]
On Thu, 4 Apr 2024 00:37:45 GMT, Vicente Romero wrote: >> Jan Lahoda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixing tests. > > src/java.compiler/share/classes/javax/lang/model/element/ElementKind.java > line 133: > >> 131: */ >> 132: >> @PreviewFeature(feature=PreviewFeature.Feature.DERIVED_RECORD_CREATION, >> reflective=true) >> 133: COMPONENT_LOCAL_VARIABLE; > > I wonder if we can't just use: LOCAL_VARIABLE We could, and I was thinking of that. But, then I decided to go with a new kind, as I think it matches better the overall design of the Elements/Kinds. E.g. we have existing `EXCEPTION_PARAMETER` (used in `catch`) or `BINDING_VARIABLE`, rather than using `LOCAL_VARIABLE` for them. (Also, many of these variable kinds have some small, but significant differences in semantics from `LOCAL_VARIABLE` - e.g. some are never blank/are always definitely assigned.) - PR Review Comment: https://git.openjdk.org/jdk/pull/18509#discussion_r1552385305
Re: RFR: 8328481: Implement Module Imports [v2]
> This is an implementation of JEP JDK-8315129: Module Import Declarations > (Preview). Please see the JEP for details: > https://bugs.openjdk.org/browse/JDK-8315129 > > It is mostly straightforward - the module imports are parsed, and then > expanded to import-on-demand in `TypeEnter`. > There is a few notable aspects, however: > - the AST node for import (`JCImport`) is holding the imported element as a > field access, because so far, the imported element always had to have a '.' > (even for import-on-demand). But for module imports, it is permissible to > import from a module whose name does not have a dot (`import module m;`). The > use of field access for ordinary import seems very useful, so I preferred to > keep that, and created a new internal-only AST node for module imports. There > is still only one public API AST node/interface, so this is purely an > implementation choice. > - JShell now supports module imports as well; and the default, implicit, > script is changed to use it to import all of `java.base` if preview is > enabled. It is expected that the default would be changed if/when the module > imports feature is finalized. Jan Lahoda 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 16 additional commits since the last revision: - Merge branch 'master' into module-imports - Merge branch 'master' into module-imports - Fixing test. - Fixing disambiguation of module imports. - Fixing file name computation. - Merge branch 'master' into module-imports - Cleanup. - Fixing CheckExamples. - Adding support for import modules to JShell. - More correct handling of requires transitive. - ... and 6 more: https://git.openjdk.org/jdk/compare/c0c0131b...3f2deab9 - Changes: - all: https://git.openjdk.org/jdk/pull/18614/files - new: https://git.openjdk.org/jdk/pull/18614/files/9cab0ae8..3f2deab9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18614=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18614=00-01 Stats: 318 lines in 18 files changed: 46 ins; 198 del; 74 mod Patch: https://git.openjdk.org/jdk/pull/18614.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18614/head:pull/18614 PR: https://git.openjdk.org/jdk/pull/18614
Re: RFR: 8328481: Implement Module Imports
On Thu, 4 Apr 2024 11:43:07 GMT, Thiago Henrique Hüpner wrote: > Should the pull request incorporate a modification to the JAVASE script file > for JShell, specifically substituting it with `import module java.se`? (right > now the imports are computed, not read from a file) Given we need to keep the existing code (as `import module java.se` would only work when preview is enabled), and given the outcomes for the user should be (I think, at least) the same, it does not seem necessary to me to complicate things by having two ways to achieve the same outcome. When the feature goes final, then maybe we could consider replacing the hardcoded JAVASE with a script. But, even then, the user might use `-C--release=21`, and `import module java.se` would not work then, so it may be easier to keep the current code even then. - PR Comment: https://git.openjdk.org/jdk/pull/18614#issuecomment-2036957025
RFR: 8328935: Implement Module Imports.
This is an implementation of JEP JDK-8315129: Module Import Declarations (Preview). Please see the JEP for details: https://bugs.openjdk.org/browse/JDK-8315129 It is mostly straightforward - the module imports are parsed, and then expanded to import-on-demand in `TypeEnter`. There is a few notable aspects, however: - the AST node for import (`JCImport`) is holding the imported element as a field access, because so far, the imported element always had to have a '.' (even for import-on-demand). But for module imports, it is permissible to import from a module whose name does not have a dot (`import module m;`). The use of field access for ordinary import seems very useful, so I preferred to keep that, and created a new internal-only AST node for module imports. There is still only one public API AST node/interface, so this is purely an implementation choice. - JShell now supports module imports as well; and the default, implicit, script is changed to use it to import all of `java.base` if preview is enabled. It is expected that the default would be changed if/when the module imports feature is finalized. - Commit messages: - Merge branch 'master' into module-imports - Fixing test. - Fixing disambiguation of module imports. - Fixing file name computation. - Merge branch 'master' into module-imports - Cleanup. - Fixing CheckExamples. - Adding support for import modules to JShell. - More correct handling of requires transitive. - Merge branch 'module-imports' of github.com:lahodaj/jdk into module-imports - ... and 5 more: https://git.openjdk.org/jdk/compare/f26e4308...9cab0ae8 Changes: https://git.openjdk.org/jdk/pull/18614/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18614=00 Issue: https://bugs.openjdk.org/browse/JDK-8328935 Stats: 1065 lines in 28 files changed: 989 ins; 11 del; 65 mod Patch: https://git.openjdk.org/jdk/pull/18614.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18614/head:pull/18614 PR: https://git.openjdk.org/jdk/pull/18614
Re: RFR: JDK-8329624: Add visitors for preview language features
On Wed, 3 Apr 2024 20:17:39 GMT, Joe Darcy wrote: > When new language features are added, the javax.lang.model may need to be > updated. For certain classes of features, the API update includes introducing > a new set of concrete visitors to handle the language feature. > > The API scaffolding to support the new feature tends to be considerably > larger than the API specifically for the new feature. > > To aid work in progress (such as https://github.com/openjdk/jdk/pull/18509) > and anticipated in the future, I think it would be helpful to introduce a > persistent set of preview visitors independent of any particular language > change. Looks good to me, thanks! - Marked as reviewed by jlahoda (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18609#pullrequestreview-1978870962
