Re: RFR: 8268124: Update java.lang to use switch expressions [v6]
> Hi, > > Could someone please review my code for updating the code in the `java.lang` > packages to make use of the switch expressions? > > Kind regards, > Patrick Patrick Concannon 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 seven additional commits since the last revision: - Merge remote-tracking branch 'origin/master' into JDK-8268124 - 8268124: moved cases into separate lines in DirectMethodHandleDescImpl part II - 8268124: moved cases onto separate lines in DirectMethodHandleDescImpl - Merge remote-tracking branch 'origin/master' into JDK-8268124 - 8268124: small refactoring; fixed misplaced comment and added missing lambda operator - Merge remote-tracking branch 'origin/master' into JDK-8268124 - 8268124: Update java.lang to use switch expressions - Changes: - all: https://git.openjdk.java.net/jdk/pull/4312/files - new: https://git.openjdk.java.net/jdk/pull/4312/files/df5b34e5..c7c11939 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4312=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4312=04-05 Stats: 4673 lines in 106 files changed: 3649 ins; 412 del; 612 mod Patch: https://git.openjdk.java.net/jdk/pull/4312.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4312/head:pull/4312 PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions [v5]
On Wed, 9 Jun 2021 10:25:35 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.lang` >> packages to make use of the switch expressions? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with two > additional commits since the last revision: > > - 8268124: moved cases into separate lines in DirectMethodHandleDescImpl > part II > - 8268124: moved cases onto separate lines in DirectMethodHandleDescImpl Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions [v5]
On Wed, 9 Jun 2021 10:25:35 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.lang` >> packages to make use of the switch expressions? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with two > additional commits since the last revision: > > - 8268124: moved cases into separate lines in DirectMethodHandleDescImpl > part II > - 8268124: moved cases onto separate lines in DirectMethodHandleDescImpl Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions [v5]
On Wed, 9 Jun 2021 10:25:35 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.lang` >> packages to make use of the switch expressions? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with two > additional commits since the last revision: > > - 8268124: moved cases into separate lines in DirectMethodHandleDescImpl > part II > - 8268124: moved cases onto separate lines in DirectMethodHandleDescImpl Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions [v5]
On Wed, 9 Jun 2021 10:25:35 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.lang` >> packages to make use of the switch expressions? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with two > additional commits since the last revision: > > - 8268124: moved cases into separate lines in DirectMethodHandleDescImpl > part II > - 8268124: moved cases onto separate lines in DirectMethodHandleDescImpl Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions [v3]
On Thu, 3 Jun 2021 17:04:58 GMT, Mandy Chung wrote:
>> Patrick Concannon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8268124: small refactoring; fixed misplaced comment and added missing
>> lambda operator
>
> src/java.base/share/classes/java/lang/constant/DirectMethodHandleDescImpl.java
> line 138:
>
>> 136: public String lookupDescriptor() {
>> 137: return switch (kind) {
>> 138: case VIRTUAL, SPECIAL,
>
> Nit: I prefer to have each case in a separate line (in this switch and also
> the switch in `resolveConstantDesc`.
Hi Mandy. Thanks for your suggestion. I've now moved each case onto its own
line. Please see ccc9db7 / df5b34e
-
PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions [v5]
> Hi, > > Could someone please review my code for updating the code in the `java.lang` > packages to make use of the switch expressions? > > Kind regards, > Patrick Patrick Concannon has updated the pull request incrementally with two additional commits since the last revision: - 8268124: moved cases into separate lines in DirectMethodHandleDescImpl part II - 8268124: moved cases onto separate lines in DirectMethodHandleDescImpl - Changes: - all: https://git.openjdk.java.net/jdk/pull/4312/files - new: https://git.openjdk.java.net/jdk/pull/4312/files/7907f3eb..df5b34e5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4312=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4312=03-04 Stats: 12 lines in 1 file changed: 6 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/4312.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4312/head:pull/4312 PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions [v4]
> Hi, > > Could someone please review my code for updating the code in the `java.lang` > packages to make use of the switch expressions? > > Kind regards, > Patrick Patrick Concannon 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 four additional commits since the last revision: - Merge remote-tracking branch 'origin/master' into JDK-8268124 - 8268124: small refactoring; fixed misplaced comment and added missing lambda operator - Merge remote-tracking branch 'origin/master' into JDK-8268124 - 8268124: Update java.lang to use switch expressions - Changes: - all: https://git.openjdk.java.net/jdk/pull/4312/files - new: https://git.openjdk.java.net/jdk/pull/4312/files/a8706b02..7907f3eb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4312=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4312=02-03 Stats: 463928 lines in 821 files changed: 455007 ins; 5063 del; 3858 mod Patch: https://git.openjdk.java.net/jdk/pull/4312.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4312/head:pull/4312 PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions [v4]
On Wed, 2 Jun 2021 16:10:19 GMT, Rémi Forax wrote: >> Patrick Concannon 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 four additional >> commits since the last revision: >> >> - Merge remote-tracking branch 'origin/master' into JDK-8268124 >> - 8268124: small refactoring; fixed misplaced comment and added missing >> lambda operator >> - Merge remote-tracking branch 'origin/master' into JDK-8268124 >> - 8268124: Update java.lang to use switch expressions > > src/java.base/share/classes/java/lang/invoke/MemberName.java line 331: > >> 329: assert(false) : this+" != >> "+MethodHandleNatives.refKindName((byte)originalRefKind); >> 330: yield true; >> 331: } > > this code always yield true, better to check if the assert are enabled, do > the switch in that case and always return true Thanks for your suggestion. I've incorporated it into commit a8706b0 - PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions [v3]
On Thu, 3 Jun 2021 11:35:13 GMT, Rémi Forax wrote: >> My mistake. I've replaced the colon now with the lambda operator. See a8706b0 > >> My mistake. I've replaced the colon now with the lambda operator. > > Drive by comment, in term of name, `->` is the arrow operator not the lambda > operator. > - lambda = parameters + arrow + code > - arrow case = case + arrow + code > > The difference is important because a lambda is a function while an arrow > case is a block. Ah OK, good to know. Thanks Remi! - PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions [v3]
On Thu, 3 Jun 2021 11:01:02 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.lang` >> packages to make use of the switch expressions? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8268124: small refactoring; fixed misplaced comment and added missing > lambda operator Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions [v3]
On Thu, 3 Jun 2021 11:01:02 GMT, Patrick Concannon
wrote:
>> Hi,
>>
>> Could someone please review my code for updating the code in the `java.lang`
>> packages to make use of the switch expressions?
>>
>> Kind regards,
>> Patrick
>
> Patrick Concannon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8268124: small refactoring; fixed misplaced comment and added missing
> lambda operator
Looks good with one minor comment in `DirectMethodHandleDescImpl.java`.
src/java.base/share/classes/java/lang/constant/DirectMethodHandleDescImpl.java
line 138:
> 136: public String lookupDescriptor() {
> 137: return switch (kind) {
> 138: case VIRTUAL, SPECIAL,
Nit: I prefer to have each case in a separate line (in this switch and also the
switch in `resolveConstantDesc`.
-
Marked as reviewed by mchung (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions [v3]
On Thu, 3 Jun 2021 11:01:02 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.lang` >> packages to make use of the switch expressions? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8268124: small refactoring; fixed misplaced comment and added missing > lambda operator LGTM - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions [v3]
On Thu, 3 Jun 2021 11:01:02 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.lang` >> packages to make use of the switch expressions? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8268124: small refactoring; fixed misplaced comment and added missing > lambda operator Looks good to me - PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions [v3]
On Thu, 3 Jun 2021 10:57:16 GMT, Patrick Concannon wrote: > My mistake. I've replaced the colon now with the lambda operator. Drive by comment, in term of name, `->` is the arrow operator not the lambda operator. - lambda = parameters + arrow + code - arrow case = case + arrow + code The difference is important because a lambda is a function while an arrow case is a block. - PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions [v3]
On Wed, 2 Jun 2021 16:06:42 GMT, Rémi Forax wrote:
>> Patrick Concannon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8268124: small refactoring; fixed misplaced comment and added missing
>> lambda operator
>
> src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
> line 1663:
>
>> 1661: case D_TYPE -> mv.visitInsn(Opcodes.DCONST_0);
>> 1662: case L_TYPE -> mv.visitInsn(Opcodes.ACONST_NULL);
>> 1663: default -> throw new InternalError("unknown type: " +
>> type);
>
> perhaps
>
> mv.visitInsn(switch(type) { ...
Hi Remi, thanks for the suggestion. I've added that in now. See commit a8706b0
> src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line
> 278:
>
>> 276: private static boolean isObjectMethod(Method m) {
>> 277: return switch (m.getName()) {
>> 278: case "toString" -> (m.getReturnType() == String.class
>
> the extra parenthesis are not needed
Parenthesis removed, as suggested. See a8706b0
> src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 366:
>
>> 364: }
>> 365: default -> throw new IllegalArgumentException(methodName);
>> 366: };
>
> I thinki it's simpler to have something like that
>
> var handle = switch(methodName) {
> ...
> };
> return methodType != null ? new ConstantCallSite(handle) : handle;
I think that looks better. I've made that change now. See a8706b0
-
PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions [v3]
> Hi, > > Could someone please review my code for updating the code in the `java.lang` > packages to make use of the switch expressions? > > Kind regards, > Patrick Patrick Concannon has updated the pull request incrementally with one additional commit since the last revision: 8268124: small refactoring; fixed misplaced comment and added missing lambda operator - Changes: - all: https://git.openjdk.java.net/jdk/pull/4312/files - new: https://git.openjdk.java.net/jdk/pull/4312/files/d705efbd..a8706b02 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4312=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4312=01-02 Stats: 52 lines in 6 files changed: 9 ins; 12 del; 31 mod Patch: https://git.openjdk.java.net/jdk/pull/4312.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4312/head:pull/4312 PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions [v3]
On Wed, 2 Jun 2021 16:43:25 GMT, Naoto Sato wrote:
>> Patrick Concannon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8268124: small refactoring; fixed misplaced comment and added missing
>> lambda operator
>
> src/java.base/share/classes/java/lang/CharacterData.java line 80:
>
>> 78: case 2 -> CharacterData02.instance;
>> 79: case 3 -> CharacterData03.instance;
>> 80: case 14 -> CharacterData0E.instance; // Private Use
>
> Plane 14 is not `private use`
Hi Naoto. Well spotted. I've corrected that now. See a8706b0
> src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java line 90:
>
>> 88: // findVirtual etc.
>> 89: return switch (refKind) {
>> 90: case REF_invokeSpecial: {
>
> Is ':' preferred here (and other cases too) instead of "->"?
My mistake. I've replaced the colon now with the lambda operator. See a8706b0
-
PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions [v2]
> Hi, > > Could someone please review my code for updating the code in the `java.lang` > packages to make use of the switch expressions? > > Kind regards, > Patrick Patrick Concannon 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 two additional commits since the last revision: - Merge remote-tracking branch 'origin/master' into JDK-8268124 - 8268124: Update java.lang to use switch expressions - Changes: - all: https://git.openjdk.java.net/jdk/pull/4312/files - new: https://git.openjdk.java.net/jdk/pull/4312/files/72c93053..d705efbd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4312=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4312=00-01 Stats: 6221 lines in 174 files changed: 4475 ins; 1291 del; 455 mod Patch: https://git.openjdk.java.net/jdk/pull/4312.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4312/head:pull/4312 PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions
On Wed, 2 Jun 2021 15:25:16 GMT, Patrick Concannon wrote: > Hi, > > Could someone please review my code for updating the code in the `java.lang` > packages to make use of the switch expressions? > > Kind regards, > Patrick Changes to Math and Long look fine. - Marked as reviewed by darcy (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions
On Wed, 2 Jun 2021 15:25:16 GMT, Patrick Concannon
wrote:
> Hi,
>
> Could someone please review my code for updating the code in the `java.lang`
> packages to make use of the switch expressions?
>
> Kind regards,
> Patrick
Hi Patrick, some minor comments.
src/java.base/share/classes/java/lang/CharacterData.java line 80:
> 78: case 2 -> CharacterData02.instance;
> 79: case 3 -> CharacterData03.instance;
> 80: case 14 -> CharacterData0E.instance; // Private Use
Plane 14 is not `private use`
src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java line 90:
> 88: // findVirtual etc.
> 89: return switch (refKind) {
> 90: case REF_invokeSpecial: {
Is ':' preferred here (and other cases too) instead of "->"?
-
PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions
On Wed, 2 Jun 2021 15:25:16 GMT, Patrick Concannon
wrote:
> Hi,
>
> Could someone please review my code for updating the code in the `java.lang`
> packages to make use of the switch expressions?
>
> Kind regards,
> Patrick
src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 366:
> 364: }
> 365: default -> throw new IllegalArgumentException(methodName);
> 366: };
I thinki it's simpler to have something like that
var handle = switch(methodName) {
...
};
return methodType != null ? new ConstantCallSite(handle) : handle;
-
PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions
On Wed, 2 Jun 2021 15:25:16 GMT, Patrick Concannon
wrote:
> Hi,
>
> Could someone please review my code for updating the code in the `java.lang`
> packages to make use of the switch expressions?
>
> Kind regards,
> Patrick
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 278:
> 276: private static boolean isObjectMethod(Method m) {
> 277: return switch (m.getName()) {
> 278: case "toString" -> (m.getReturnType() == String.class
the extra parenthesis are not needed
-
PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions
On Wed, 2 Jun 2021 15:25:16 GMT, Patrick Concannon wrote: > Hi, > > Could someone please review my code for updating the code in the `java.lang` > packages to make use of the switch expressions? > > Kind regards, > Patrick src/java.base/share/classes/java/lang/invoke/MemberName.java line 331: > 329: assert(false) : this+" != > "+MethodHandleNatives.refKindName((byte)originalRefKind); > 330: yield true; > 331: } this code always yield true, better to check if the assert are enabled, do the switch in that case and always return true - PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions
On Wed, 2 Jun 2021 15:25:16 GMT, Patrick Concannon
wrote:
> Hi,
>
> Could someone please review my code for updating the code in the `java.lang`
> packages to make use of the switch expressions?
>
> Kind regards,
> Patrick
src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line
1663:
> 1661: case D_TYPE -> mv.visitInsn(Opcodes.DCONST_0);
> 1662: case L_TYPE -> mv.visitInsn(Opcodes.ACONST_NULL);
> 1663: default -> throw new InternalError("unknown type: " + type);
perhaps
mv.visitInsn(switch(type) { ...
-
PR: https://git.openjdk.java.net/jdk/pull/4312
RFR: 8268124: Update java.lang to use switch expressions
Hi, Could someone please review my code for updating the code in the `java.lang` packages to make use of the switch expressions? Kind regards, Patrick - Commit messages: - 8268124: Update java.lang to use switch expressions Changes: https://git.openjdk.java.net/jdk/pull/4312/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4312=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268124 Stats: 551 lines in 22 files changed: 7 ins; 140 del; 404 mod Patch: https://git.openjdk.java.net/jdk/pull/4312.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4312/head:pull/4312 PR: https://git.openjdk.java.net/jdk/pull/4312
