Re: RFR: 8268083: JDK-8267706 breaks bin/idea.sh on a Mac
On Fri, 4 Jun 2021 21:23:27 GMT, Nikita Gubarkov wrote: > I got rid of `realpath` usage as discussed in > https://github.com/openjdk/jdk/pull/4190 and used `RelativePath` macro > instead, however there were quite a few problems with this macro, here's the > example: > > $(call RelativePath,/foo/bar,/foo/bar/baz) -> "..//foo/bar" > $(call RelativePath,/foo/bar/baz/,/foo/bar/baz) -> SEGFAULT > $(call RelativePath,/foo/bar/baz/banan,/foo/bar/) -> "./baz/banan" > $(call RelativePath,/foo/bar/baz,/foo/bar/banan) -> "../baz" > > As you can see, 1st case is just plain wrong, 2nd crashes make because of > infinite loop, 3rd can be simplified and all of them have leading whitespaces > First commit in this PR fixes all these issues and adds corresponding test > cases and second commit replaces usage of `realpath` in idea.sh with > `RelativePath` macro in idea.gmk and fixes problems, when paths are > incorrectly treated by IDEA Nice work, this looks great! Thank you for improving the RelativePath macro. Since this is touching some very core and sensitive build functionality, I'm going to run it through all our internal builds just to be safe. - PR: https://git.openjdk.java.net/jdk/pull/4369
RFR: 8268083: JDK-8267706 breaks bin/idea.sh on a Mac
I got rid of `realpath` usage as discussed in https://github.com/openjdk/jdk/pull/4190 and used `RelativePath` macro instead, however there were quite a few problems with this macro, here's the example: $(call RelativePath,/foo/bar,/foo/bar/baz) -> "..//foo/bar" $(call RelativePath,/foo/bar/baz/,/foo/bar/baz) -> SEGFAULT $(call RelativePath,/foo/bar/baz/banan,/foo/bar/) -> "./baz/banan" $(call RelativePath,/foo/bar/baz,/foo/bar/banan) -> "../baz" As you can see, 1st case is just plain wrong, 2nd crashes make because of infinite loop, 3rd can be simplified and all of them have leading whitespaces First commit in this PR fixes all these issues and adds corresponding test cases and second commit replaces usage of `realpath` in idea.sh with `RelativePath` macro in idea.gmk and fixes problems, when paths are incorrectly treated by IDEA - Commit messages: - 8268083: Got rid of realpath usage in bin/idea.sh - 8268083: Fix FindCommonPathPrefix and RelativePath macros in make/common/Utils.gmk Changes: https://git.openjdk.java.net/jdk/pull/4369/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4369=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268083 Stats: 219 lines in 11 files changed: 107 ins; 47 del; 65 mod Patch: https://git.openjdk.java.net/jdk/pull/4369.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4369/head:pull/4369 PR: https://git.openjdk.java.net/jdk/pull/4369
RFR: JDK-8263389 IGV: Zooming changes the point that is currently centered
Fixing the zoom issue on IGV. I tested this on Windows and Linux and zooming now works as expected. - Commit messages: - IGV: Zooming now preserves the point currently centered Changes: https://git.openjdk.java.net/jdk/pull/4291/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4291=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8263389 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4291.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4291/head:pull/4291 PR: https://git.openjdk.java.net/jdk/pull/4291
Re: RFR: JDK-8263389 IGV: Zooming changes the point that is currently centered
On Tue, 1 Jun 2021 20:00:07 GMT, jtfuller111 wrote: > Fixing the zoom issue on IGV. I tested this on Windows and Linux and zooming > now works as expected. Hi, please send an e-mail to dalibor.to...@oracle.com so that I can mark your account as verified. - PR: https://git.openjdk.java.net/jdk/pull/4291
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]
On Fri, 4 Jun 2021 18:23:28 GMT, Vicente Romero wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixing typo.
>
> test/langtools/tools/javac/patterns/SealedTypeChanges.java line 71:
>
>> 69: }
>> 70:
>> 71: sealed interface SealedTypeChangesIntf permits SealedTypeChanges.A {}
>
> just for completeness shouldn't we have a test with sealed, non-abstract
> classes?
Note that for sealed non-abstract classes, the permits is not checked (as an
instance of the non-abstract class may be created and passed to the switch, the
switch needs to contain a case that will cover the class anyway). I've added
tests that check the behavior for abstract class, and non-abstract classes
(error is produced in the latter case).
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]
On Fri, 4 Jun 2021 15:46:32 GMT, Paul Sandoz wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixing typo.
>
> test/langtools/tools/javac/patterns/DisambiguateParenthesizedPattern.java
> line 72:
>
>> 70: SwitchTree st = (SwitchTree)
>> method.getBody().getStatements().get(0);
>> 71: CaseLabelTree label = st.getCases().get(0).getLabels().get(0);
>> 72: ExpressionType actualType = switch (label) {
>
> Should the test be careful of using a pattern match switch?
I don't think using the new feature in the tests is problematic (esp. javac
tests related to the feature). It helps to ensure the feature really works on
real code.
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v13]
> This is a preview of a patch implementing JEP 406: Pattern Matching for > switch (Preview): > https://bugs.openjdk.java.net/browse/JDK-8213076 > > The current draft of the specification is here: > http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html > > A summary of notable parts of the patch: > -to support cases expressions and patterns in cases, there is a new common > superinterface for expressions and patterns, `CaseLabelTree`, which > expressions and patterns implement, and a list of case labels is returned > from `CaseTree.getLabels()`. > -to support `case default`, there is an implementation of `CaseLabelTree` > that represents it (`DefaultCaseLabelTree`). It is used also to represent the > conventional `default` internally and in the newly added methods. > -in the parser, parenthesized patterns and expressions need to be > disambiguated when parsing case labels. > -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive, > String, enum) switches. This is a bit tricky for boxed primitives, as there > is no value that is not part of the input domain so that could be used to > represent `case null`. Requires a bit shuffling with values. > -TransPatterns has been enhanced to handle the pattern matching switch. It > produces code that delegates to a new bootstrap method, that will classify > the input value to the switch and return the case number, to which the switch > then jumps. To support guards, the switches (and the bootstrap method) are > restartable. The bootstrap method as such is written very simply so far, but > could be much more optimized later. > -nullable type patterns are `case String s, null`/`case null, String s`/`case > null: case String s:`/`case String s: case null:`, handling of these required > a few tricks in `Attr`, `Flow` and `TransPatterns`. > > The specdiff for the change is here (to be updated): > http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html Jan Lahoda has updated the pull request incrementally with two additional commits since the last revision: - Applying review feedback. - Tweaking javadoc. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3863/files - new: https://git.openjdk.java.net/jdk/pull/3863/files/8d4c02b4..e3c29759 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3863=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3863=11-12 Stats: 125 lines in 8 files changed: 91 ins; 10 del; 24 mod Patch: https://git.openjdk.java.net/jdk/pull/3863.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3863/head:pull/3863 PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]
On Fri, 4 Jun 2021 09:46:31 GMT, Jan Lahoda wrote: >> This is a preview of a patch implementing JEP 406: Pattern Matching for >> switch (Preview): >> https://bugs.openjdk.java.net/browse/JDK-8213076 >> >> The current draft of the specification is here: >> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html >> >> A summary of notable parts of the patch: >> -to support cases expressions and patterns in cases, there is a new common >> superinterface for expressions and patterns, `CaseLabelTree`, which >> expressions and patterns implement, and a list of case labels is returned >> from `CaseTree.getLabels()`. >> -to support `case default`, there is an implementation of `CaseLabelTree` >> that represents it (`DefaultCaseLabelTree`). It is used also to represent >> the conventional `default` internally and in the newly added methods. >> -in the parser, parenthesized patterns and expressions need to be >> disambiguated when parsing case labels. >> -Lower has been enhanced to handle `case null` for ordinary >> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed >> primitives, as there is no value that is not part of the input domain so >> that could be used to represent `case null`. Requires a bit shuffling with >> values. >> -TransPatterns has been enhanced to handle the pattern matching switch. It >> produces code that delegates to a new bootstrap method, that will classify >> the input value to the switch and return the case number, to which the >> switch then jumps. To support guards, the switches (and the bootstrap >> method) are restartable. The bootstrap method as such is written very simply >> so far, but could be much more optimized later. >> -nullable type patterns are `case String s, null`/`case null, String >> s`/`case null: case String s:`/`case String s: case null:`, handling of >> these required a few tricks in `Attr`, `Flow` and `TransPatterns`. >> >> The specdiff for the change is here (to be updated): >> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Fixing typo. Thanks a lot for all the feedback. I've tried to do the requested changes in the recent commits. - PR: https://git.openjdk.java.net/jdk/pull/3863
Integrated: 8268272: Remove JDK-8264874 changes because Graal was removed.
On Fri, 4 Jun 2021 17:40:48 GMT, Vladimir Kozlov wrote: > JDK-8264806 removed Graal sources from JDK and INCLUDE_GRAAL is not defined > anymore. We can now remove code added by JDK-8264874: > https://github.com/openjdk/jdk/commit/951f277a > > Tested Tier1. This pull request has now been integrated. Changeset: 48dc72b7 Author:Vladimir Kozlov URL: https://git.openjdk.java.net/jdk/commit/48dc72b74d6b4b7b8fb605b62fc0057b5f4652e1 Stats: 3 lines in 1 file changed: 0 ins; 3 del; 0 mod 8268272: Remove JDK-8264874 changes because Graal was removed. Reviewed-by: erikj - PR: https://git.openjdk.java.net/jdk/pull/4367
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]
On Fri, 4 Jun 2021 09:46:31 GMT, Jan Lahoda wrote:
>> This is a preview of a patch implementing JEP 406: Pattern Matching for
>> switch (Preview):
>> https://bugs.openjdk.java.net/browse/JDK-8213076
>>
>> The current draft of the specification is here:
>> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>>
>> A summary of notable parts of the patch:
>> -to support cases expressions and patterns in cases, there is a new common
>> superinterface for expressions and patterns, `CaseLabelTree`, which
>> expressions and patterns implement, and a list of case labels is returned
>> from `CaseTree.getLabels()`.
>> -to support `case default`, there is an implementation of `CaseLabelTree`
>> that represents it (`DefaultCaseLabelTree`). It is used also to represent
>> the conventional `default` internally and in the newly added methods.
>> -in the parser, parenthesized patterns and expressions need to be
>> disambiguated when parsing case labels.
>> -Lower has been enhanced to handle `case null` for ordinary
>> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed
>> primitives, as there is no value that is not part of the input domain so
>> that could be used to represent `case null`. Requires a bit shuffling with
>> values.
>> -TransPatterns has been enhanced to handle the pattern matching switch. It
>> produces code that delegates to a new bootstrap method, that will classify
>> the input value to the switch and return the case number, to which the
>> switch then jumps. To support guards, the switches (and the bootstrap
>> method) are restartable. The bootstrap method as such is written very simply
>> so far, but could be much more optimized later.
>> -nullable type patterns are `case String s, null`/`case null, String
>> s`/`case null: case String s:`/`case String s: case null:`, handling of
>> these required a few tricks in `Attr`, `Flow` and `TransPatterns`.
>>
>> The specdiff for the change is here (to be updated):
>> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixing typo.
test/langtools/tools/javac/patterns/SealedTypeChanges.java line 58:
> 56: void statement(SealedTypeChangesIntf obj) {
> 57: switch (obj) {
> 58: case A a -> System.err.println(1);
what about having tests with a case that matches the sealed class?
test/langtools/tools/javac/patterns/SealedTypeChanges.java line 71:
> 69: }
> 70:
> 71: sealed interface SealedTypeChangesIntf permits SealedTypeChanges.A {}
just for completeness shouldn't we have a test with sealed, non-abstract
classes?
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8268272: Remove JDK-8264874 changes because Graal was removed.
On Fri, 4 Jun 2021 17:40:48 GMT, Vladimir Kozlov wrote: > JDK-8264806 removed Graal sources from JDK and INCLUDE_GRAAL is not defined > anymore. We can now remove code added by JDK-8264874: > https://github.com/openjdk/jdk/commit/951f277a > > Tested Tier1. Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4367
RFR: 8268272: Remove JDK-8264874 changes because Graal was removed.
JDK-8264806 removed Graal sources from JDK and INCLUDE_GRAAL is not defined anymore. We can now remove code added by JDK-8264874: https://github.com/openjdk/jdk/commit/951f277a Tested Tier1. - Commit messages: - 8268272: Remove JDK-8264874 changes because Graal was removed. Changes: https://git.openjdk.java.net/jdk/pull/4367/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4367=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268272 Stats: 3 lines in 1 file changed: 0 ins; 3 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4367.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4367/head:pull/4367 PR: https://git.openjdk.java.net/jdk/pull/4367
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]
On Fri, 4 Jun 2021 09:46:31 GMT, Jan Lahoda wrote:
>> This is a preview of a patch implementing JEP 406: Pattern Matching for
>> switch (Preview):
>> https://bugs.openjdk.java.net/browse/JDK-8213076
>>
>> The current draft of the specification is here:
>> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>>
>> A summary of notable parts of the patch:
>> -to support cases expressions and patterns in cases, there is a new common
>> superinterface for expressions and patterns, `CaseLabelTree`, which
>> expressions and patterns implement, and a list of case labels is returned
>> from `CaseTree.getLabels()`.
>> -to support `case default`, there is an implementation of `CaseLabelTree`
>> that represents it (`DefaultCaseLabelTree`). It is used also to represent
>> the conventional `default` internally and in the newly added methods.
>> -in the parser, parenthesized patterns and expressions need to be
>> disambiguated when parsing case labels.
>> -Lower has been enhanced to handle `case null` for ordinary
>> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed
>> primitives, as there is no value that is not part of the input domain so
>> that could be used to represent `case null`. Requires a bit shuffling with
>> values.
>> -TransPatterns has been enhanced to handle the pattern matching switch. It
>> produces code that delegates to a new bootstrap method, that will classify
>> the input value to the switch and return the case number, to which the
>> switch then jumps. To support guards, the switches (and the bootstrap
>> method) are restartable. The bootstrap method as such is written very simply
>> so far, but could be much more optimized later.
>> -nullable type patterns are `case String s, null`/`case null, String
>> s`/`case null: case String s:`/`case String s: case null:`, handling of
>> these required a few tricks in `Attr`, `Flow` and `TransPatterns`.
>>
>> The specdiff for the change is here (to be updated):
>> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixing typo.
I reviewed the `java.base` change namely, SwitchBootstraps.java. Looks good.
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 47:
> 45: * of the {@code switch}, implicitly numbered sequentially from {@code
> [0..N)}.
> 46: *
> 47: * The bootstrap call site accepts a single parameter of the type of
> the
It takes 2 parameters (not single parameter). Perhaps you can take out this
paragraph since it's specified in the `typeSwitch` method.
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 110:
> 108: * @return a {@code CallSite} returning the first matching element
> as described above
> 109: *
> 110: * @throws NullPointerException if any argument is null
Suggestion:
* @throws NullPointerException if any argument is {@code null}
same formatting nit for other occurrenace of "null"
-
Marked as reviewed by mchung (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8268267: Remove -Djavatest.security.noSecurityManager=true from jtreg runs
On Fri, 4 Jun 2021 15:53:42 GMT, Weijun Wang wrote: > Now that the default behavior of JDK 17 is still > `-Djava.security.manager=allow`, we can remove the > `-Djavatest.security.noSecurityManager=true` option from the jtreg command > line inside `RunTests.gmk`. Three problem-listed langtools tests can also be > liberated. Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4364
Re: RFR: 8268267: Remove -Djavatest.security.noSecurityManager=true from jtreg runs
On Fri, 4 Jun 2021 16:10:02 GMT, Jonathan Gibbons wrote: >> Now that the default behavior of JDK 17 is still >> `-Djava.security.manager=allow`, we can remove the >> `-Djavatest.security.noSecurityManager=true` option from the jtreg command >> line inside `RunTests.gmk`. Three problem-listed langtools tests can also be >> liberated. > > test/langtools/ProblemList.txt line 51: > >> 49: tools/javac/warnings/suppress/TypeAnnotations.java >>8057683generic-allimprove ordering of errors with type >> annotations >> 50: tools/javac/modules/SourceInSymlinkTest.java >>8180263windows-allfails when run on a subst drive >> 51: tools/javac/processing/options/XprintRepeatingAnnotations.java >>8265611generic-all@compile/ref comparison fails when >> noSecurityManager=true > > Maybe close out JDK-8265611? Since you asked, I will close them as "cannot reproduce". :-) Update: "not an issue" used instead. - PR: https://git.openjdk.java.net/jdk/pull/4364
Re: RFR: 8268267: Remove -Djavatest.security.noSecurityManager=true from jtreg runs
On Fri, 4 Jun 2021 15:53:42 GMT, Weijun Wang wrote: > Now that the default behavior of JDK 17 is still > `-Djava.security.manager=allow`, we can remove the > `-Djavatest.security.noSecurityManager=true` option from the jtreg command > line inside `RunTests.gmk`. Three problem-listed langtools tests can also be > liberated. Marked as reviewed by jjg (Reviewer). test/langtools/ProblemList.txt line 51: > 49: tools/javac/warnings/suppress/TypeAnnotations.java > 8057683generic-allimprove ordering of errors with type > annotations > 50: tools/javac/modules/SourceInSymlinkTest.java > 8180263windows-allfails when run on a subst drive > 51: tools/javac/processing/options/XprintRepeatingAnnotations.java > 8265611generic-all@compile/ref comparison fails when > noSecurityManager=true Maybe close out JDK-8265611? - PR: https://git.openjdk.java.net/jdk/pull/4364
RFR: 8268267: Remove -Djavatest.security.noSecurityManager=true from jtreg runs
Now that the default behavior of JDK 17 is still `-Djava.security.manager=allow`, we can remove the `-Djavatest.security.noSecurityManager=true` option from the jtreg command line inside `RunTests.gmk`. Three problem-listed langtools tests can also be liberated. - Commit messages: - 8268267: Remove -Djavatest.security.noSecurityManager=true from jtreg runs Changes: https://git.openjdk.java.net/jdk/pull/4364/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4364=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268267 Stats: 4 lines in 2 files changed: 0 ins; 3 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4364.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4364/head:pull/4364 PR: https://git.openjdk.java.net/jdk/pull/4364
Re: RFR: 8268267: Remove -Djavatest.security.noSecurityManager=true from jtreg runs
On Fri, 4 Jun 2021 15:53:42 GMT, Weijun Wang wrote: > Now that the default behavior of JDK 17 is still > `-Djava.security.manager=allow`, we can remove the > `-Djavatest.security.noSecurityManager=true` option from the jtreg command > line inside `RunTests.gmk`. Three problem-listed langtools tests can also be > liberated. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4364
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]
On Fri, 4 Jun 2021 09:46:31 GMT, Jan Lahoda wrote:
>> This is a preview of a patch implementing JEP 406: Pattern Matching for
>> switch (Preview):
>> https://bugs.openjdk.java.net/browse/JDK-8213076
>>
>> The current draft of the specification is here:
>> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>>
>> A summary of notable parts of the patch:
>> -to support cases expressions and patterns in cases, there is a new common
>> superinterface for expressions and patterns, `CaseLabelTree`, which
>> expressions and patterns implement, and a list of case labels is returned
>> from `CaseTree.getLabels()`.
>> -to support `case default`, there is an implementation of `CaseLabelTree`
>> that represents it (`DefaultCaseLabelTree`). It is used also to represent
>> the conventional `default` internally and in the newly added methods.
>> -in the parser, parenthesized patterns and expressions need to be
>> disambiguated when parsing case labels.
>> -Lower has been enhanced to handle `case null` for ordinary
>> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed
>> primitives, as there is no value that is not part of the input domain so
>> that could be used to represent `case null`. Requires a bit shuffling with
>> values.
>> -TransPatterns has been enhanced to handle the pattern matching switch. It
>> produces code that delegates to a new bootstrap method, that will classify
>> the input value to the switch and return the case number, to which the
>> switch then jumps. To support guards, the switches (and the bootstrap
>> method) are restartable. The bootstrap method as such is written very simply
>> so far, but could be much more optimized later.
>> -nullable type patterns are `case String s, null`/`case null, String
>> s`/`case null: case String s:`/`case String s: case null:`, handling of
>> these required a few tricks in `Attr`, `Flow` and `TransPatterns`.
>>
>> The specdiff for the change is here (to be updated):
>> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixing typo.
A really nice feature, and a significant amount of work in javac. I mostly
focused on the bootstrap and API aspects, and left some minor comments (most of
which you can choose to apply or not as you see fit).
I suspect the bootstrap might evolve as we get feedback and switch is enhanced
with further forms of matching. But, at the moment it looks good.
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 87:
> 85: * returns {@literal -1}.
> 86: *
> 87: * the {@code target} is not {@code null}, then the method of the
> call site
Suggestion:
* If the {@code target} is not {@code null}, then the method of the call
site
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 92:
> 90: *
> 91: * the element is of type {@code Class} and the target value
> 92: * is a subtype of this {@code Class}; or
Suggestion:
* the element is of type {@code Class} that is assignable
* from the target's class; or
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 162:
> 160: return i;
> 161: } else {
> 162: if (label instanceof Integer constant) {
Minor suggestion: use `else if` rather than nest
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 166:
> 164: return i;
> 165: }
> 166: if (target instanceof Character input &&
> constant.intValue() == input.charValue()) {
Use `else if`
src/jdk.compiler/share/classes/com/sun/source/tree/CaseLabelTree.java line 31:
> 29:
> 30: /**A marker interface for {@code Tree}s that may be used as {@link
> CaseTree} labels.
> 31: *
Suggestion:
/**
* A marker interface for {@code Tree}s that may be used as {@link CaseTree}
labels.
*
src/jdk.compiler/share/classes/com/sun/source/tree/DefaultCaseLabelTree.java
line 30:
> 28:
> 29: /** A case label that marks {@code default} in {@code case null, default}.
> 30: * @since 17
Suggestion:
/**
* A case label that marks {@code default} in {@code case null, default}.
*
* @since 17
test/jdk/java/lang/runtime/SwitchBootstrapsTest.java line 55:
> 53: catch (NoSuchMethodException | IllegalAccessException e) {
> 54: throw new RuntimeException(e);
> 55: }
Suggestion:
catch (ReflectiveOperationException e) {
throw new AssertionError(e, "Should not happen");
}
test/jdk/java/lang/runtime/SwitchBootstrapsTest.java line 73:
> 71: }
> 72:
> 73: public void testTypes() throws Throwable {
As a follow up issue consider adding tests for `null` values
Integrated: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries
On Wed, 2 Jun 2021 17:19:06 GMT, Maurizio Cimadamore wrote: > This patch overhauls the library loading mechanism used by the Foreign Linker > API. We realized that, while handy, the *default* lookup abstraction > (`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms. > > This patch replaces `LibraryLookup` with a simpler `SymbolLookup` > abstraction, a functional interface. Crucially, `SymbolLookup` does not > concern with library loading, only symbol lookup. For this reason, two > factories are added: > > * `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to > lookup symbols in libraries loaded by current loader > * `CLinker::systemLookup` - a more stable replacement for the *default* > lookup, which looks for symbols in libc.so (or its equivalent in other > platforms). The contents of this lookup are unspecified. > > Both factories are *restricted*, so they can only be called when > `--enable-native-access` is set. This pull request has now been integrated. Changeset: 59a539fe Author:Maurizio Cimadamore URL: https://git.openjdk.java.net/jdk/commit/59a539fef12dec6ba8af8a41000829402e7e9b72 Stats: 1351 lines in 47 files changed: 626 ins; 621 del; 104 mod 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries Reviewed-by: jvernee, psandoz - PR: https://git.openjdk.java.net/jdk/pull/4316
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]
> This is a preview of a patch implementing JEP 406: Pattern Matching for > switch (Preview): > https://bugs.openjdk.java.net/browse/JDK-8213076 > > The current draft of the specification is here: > http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html > > A summary of notable parts of the patch: > -to support cases expressions and patterns in cases, there is a new common > superinterface for expressions and patterns, `CaseLabelTree`, which > expressions and patterns implement, and a list of case labels is returned > from `CaseTree.getLabels()`. > -to support `case default`, there is an implementation of `CaseLabelTree` > that represents it (`DefaultCaseLabelTree`). It is used also to represent the > conventional `default` internally and in the newly added methods. > -in the parser, parenthesized patterns and expressions need to be > disambiguated when parsing case labels. > -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive, > String, enum) switches. This is a bit tricky for boxed primitives, as there > is no value that is not part of the input domain so that could be used to > represent `case null`. Requires a bit shuffling with values. > -TransPatterns has been enhanced to handle the pattern matching switch. It > produces code that delegates to a new bootstrap method, that will classify > the input value to the switch and return the case number, to which the switch > then jumps. To support guards, the switches (and the bootstrap method) are > restartable. The bootstrap method as such is written very simply so far, but > could be much more optimized later. > -nullable type patterns are `case String s, null`/`case null, String s`/`case > null: case String s:`/`case String s: case null:`, handling of these required > a few tricks in `Attr`, `Flow` and `TransPatterns`. > > The specdiff for the change is here (to be updated): > http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Fixing typo. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3863/files - new: https://git.openjdk.java.net/jdk/pull/3863/files/216b87c2..8d4c02b4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3863=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3863=10-11 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3863.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3863/head:pull/3863 PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v11]
On Fri, 4 Jun 2021 09:01:27 GMT, Jan Lahoda wrote: >> This is a preview of a patch implementing JEP 406: Pattern Matching for >> switch (Preview): >> https://bugs.openjdk.java.net/browse/JDK-8213076 >> >> The current draft of the specification is here: >> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html >> >> A summary of notable parts of the patch: >> -to support cases expressions and patterns in cases, there is a new common >> superinterface for expressions and patterns, `CaseLabelTree`, which >> expressions and patterns implement, and a list of case labels is returned >> from `CaseTree.getLabels()`. >> -to support `case default`, there is an implementation of `CaseLabelTree` >> that represents it (`DefaultCaseLabelTree`). It is used also to represent >> the conventional `default` internally and in the newly added methods. >> -in the parser, parenthesized patterns and expressions need to be >> disambiguated when parsing case labels. >> -Lower has been enhanced to handle `case null` for ordinary >> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed >> primitives, as there is no value that is not part of the input domain so >> that could be used to represent `case null`. Requires a bit shuffling with >> values. >> -TransPatterns has been enhanced to handle the pattern matching switch. It >> produces code that delegates to a new bootstrap method, that will classify >> the input value to the switch and return the case number, to which the >> switch then jumps. To support guards, the switches (and the bootstrap >> method) are restartable. The bootstrap method as such is written very simply >> so far, but could be much more optimized later. >> -nullable type patterns are `case String s, null`/`case null, String >> s`/`case null: case String s:`/`case String s: case null:`, handling of >> these required a few tricks in `Attr`, `Flow` and `TransPatterns`. >> >> The specdiff for the change is here (to be updated): >> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html > > Jan Lahoda has updated the pull request incrementally with two additional > commits since the last revision: > > - Tweaking SwitchBootstraps javadoc, as suggested. > - Improving javadoc. Re-approving to keep the bots happy - Marked as reviewed by mcimadamore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v11]
> This is a preview of a patch implementing JEP 406: Pattern Matching for > switch (Preview): > https://bugs.openjdk.java.net/browse/JDK-8213076 > > The current draft of the specification is here: > http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html > > A summary of notable parts of the patch: > -to support cases expressions and patterns in cases, there is a new common > superinterface for expressions and patterns, `CaseLabelTree`, which > expressions and patterns implement, and a list of case labels is returned > from `CaseTree.getLabels()`. > -to support `case default`, there is an implementation of `CaseLabelTree` > that represents it (`DefaultCaseLabelTree`). It is used also to represent the > conventional `default` internally and in the newly added methods. > -in the parser, parenthesized patterns and expressions need to be > disambiguated when parsing case labels. > -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive, > String, enum) switches. This is a bit tricky for boxed primitives, as there > is no value that is not part of the input domain so that could be used to > represent `case null`. Requires a bit shuffling with values. > -TransPatterns has been enhanced to handle the pattern matching switch. It > produces code that delegates to a new bootstrap method, that will classify > the input value to the switch and return the case number, to which the switch > then jumps. To support guards, the switches (and the bootstrap method) are > restartable. The bootstrap method as such is written very simply so far, but > could be much more optimized later. > -nullable type patterns are `case String s, null`/`case null, String s`/`case > null: case String s:`/`case String s: case null:`, handling of these required > a few tricks in `Attr`, `Flow` and `TransPatterns`. > > The specdiff for the change is here (to be updated): > http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html Jan Lahoda has updated the pull request incrementally with two additional commits since the last revision: - Tweaking SwitchBootstraps javadoc, as suggested. - Improving javadoc. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3863/files - new: https://git.openjdk.java.net/jdk/pull/3863/files/fa50b5fb..216b87c2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3863=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3863=09-10 Stats: 66 lines in 2 files changed: 40 ins; 9 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/3863.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3863/head:pull/3863 PR: https://git.openjdk.java.net/jdk/pull/3863
