Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v2]
On Tue, 25 May 2021 15:18:46 GMT, Chris Hegarty 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 three additional
>> commits since the last revision:
>>
>> - 8267670: Updated code to use yield
>> - Merge remote-tracking branch 'origin/master' into JDK-8267670
>> - 8267670: Update java.io, java.math, and java.text to use switch
>> expressions
>
> src/java.base/share/classes/java/io/ObjectStreamClass.java line 2172:
>
>> 2170: switch (typeCodes[i]) {
>> 2171: case 'L', '[' -> vals[offsets[i]] =
>> unsafe.getReference(obj, readKeys[i]);
>> 2172: default -> throw new InternalError();
>
> suggest:
>
> vals[offsets[i]] = switch (typeCodes[i]) {
> case 'L', '[' -> unsafe.getReference(obj, readKeys[i]);
> default -> throw new InternalError();
Comment addressed in e503ed2
> src/java.base/share/classes/java/io/StreamTokenizer.java line 787:
>
>> 785: case TT_WORD-> ret = sval;
>> 786: case TT_NUMBER -> ret = "n=" + nval;
>> 787: case TT_NOTHING -> ret = "NOTHING";
>
> This is not correct, the assignments to ret in these case arms is redundant.
Comment addressed in e503ed2
-
PR: https://git.openjdk.java.net/jdk/pull/4182
Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v2]
On Tue, 25 May 2021 14:57:22 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.io`, >> `java.math`, and `java.text` 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 three additional > commits since the last revision: > > - 8267670: Updated code to use yield > - Merge remote-tracking branch 'origin/master' into JDK-8267670 > - 8267670: Update java.io, java.math, and java.text to use switch expressions Changes in java.math look fine. - Marked as reviewed by darcy (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4182
Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v2]
On Tue, 25 May 2021 14:53:44 GMT, Patrick Concannon
wrote:
>> src/java.base/share/classes/java/io/StreamTokenizer.java line 795:
>>
>>> 793: * case statements
>>> 794: */
>>> 795: if (ttype < 256 && ((ctype[ttype] & CT_QUOTE) != 0)) {
>>
>> Maybe (since its easier to grok the yield rather than the assignment of ret
>> in branches):
>>
>> String ret = switch (ttype) {
>> case TT_EOF -> "EOF";
>> case TT_EOL -> "EOL";
>> case TT_WORD-> sval;
>> case TT_NUMBER -> "n=" + nval;
>> case TT_NOTHING -> "NOTHING";
>> default -> {
>> /*
>> * ttype is the first character of either a quoted string or
>> * is an ordinary character. ttype can definitely not be less
>> * than 0, since those are reserved values used in the
>> previous
>> * case statements
>> */
>> if (ttype < 256 && ((ctype[ttype] & CT_QUOTE) != 0)) {
>> yield sval;
>> }
>> char s[] = new char[3];
>> s[0] = s[2] = ''';
>> s[1] = (char) ttype;
>> yield new String(s);
>> }
>> };
>> return "Token[" + ret + "], line " + LINENO;
>
> Code updated as suggested. See adc8af4
The snippet above both uses yield in the default case, and also removes the
assignments from the other arms. adc8af4 overlooks the redundant assignments in
the non-default cases.
-
PR: https://git.openjdk.java.net/jdk/pull/4182
Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v2]
On Tue, 25 May 2021 14:57:22 GMT, Patrick Concannon
wrote:
>> Hi,
>>
>> Could someone please review my code for updating the code in the `java.io`,
>> `java.math`, and `java.text` 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 three additional
> commits since the last revision:
>
> - 8267670: Updated code to use yield
> - Merge remote-tracking branch 'origin/master' into JDK-8267670
> - 8267670: Update java.io, java.math, and java.text to use switch expressions
src/java.base/share/classes/java/io/ObjectStreamClass.java line 2172:
> 2170: switch (typeCodes[i]) {
> 2171: case 'L', '[' -> vals[offsets[i]] =
> unsafe.getReference(obj, readKeys[i]);
> 2172: default -> throw new InternalError();
suggest:
vals[offsets[i]] = switch (typeCodes[i]) {
case 'L', '[' -> unsafe.getReference(obj, readKeys[i]);
default -> throw new InternalError();
src/java.base/share/classes/java/io/StreamTokenizer.java line 787:
> 785: case TT_WORD-> ret = sval;
> 786: case TT_NUMBER -> ret = "n=" + nval;
> 787: case TT_NOTHING -> ret = "NOTHING";
This is not correct, the assignments to ret in these case arms is redundant.
-
PR: https://git.openjdk.java.net/jdk/pull/4182
Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v2]
On Tue, 25 May 2021 12:43:00 GMT, Naoto Sato 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 three additional >> commits since the last revision: >> >> - 8267670: Updated code to use yield >> - Merge remote-tracking branch 'origin/master' into JDK-8267670 >> - 8267670: Update java.io, java.math, and java.text to use switch >> expressions > > src/java.base/share/classes/java/text/BreakIterator.java line 569: > >> 567: case SENTENCE_INDEX -> >> breakIteratorProvider.getSentenceInstance(locale); >> 568: default -> null; >> 569: }; > > No need to use the local variable `iterator` here. Simply return with the > switch expression. Hi Naoto, thanks for spotting this. Code updated as suggested. See adc8af4 > src/java.base/share/classes/java/text/NumberFormat.java line 982: > >> 980: case COMPACTSTYLE -> >> provider.getCompactNumberInstance(locale, formatStyle); >> 981: default-> null; >> 982: }; > > Same as `BreakIterator`. No need for `numberFormat`. Code updated as suggested. See adc8af4 - PR: https://git.openjdk.java.net/jdk/pull/4182
Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v2]
On Tue, 25 May 2021 12:31:38 GMT, Chris Hegarty 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 three additional
>> commits since the last revision:
>>
>> - 8267670: Updated code to use yield
>> - Merge remote-tracking branch 'origin/master' into JDK-8267670
>> - 8267670: Update java.io, java.math, and java.text to use switch
>> expressions
>
> src/java.base/share/classes/java/io/ObjectInputStream.java line 1877:
>
>> 1875: descriptor.checkInitialized();
>> 1876: }
>> 1877: default -> throw new
>> StreamCorruptedException(
>
> What would you think of assigning descriptor with the value returned from
> evaluating the switch?
>
> Either:
>
> 1)
>
> ObjectStreamClass descriptor = switch (tc) {
> case TC_NULL-> (ObjectStreamClass) readNull();
> case TC_PROXYCLASSDESC -> readProxyDesc(unshared);
> case TC_CLASSDESC -> readNonProxyDesc(unshared);
> case TC_REFERENCE -> readAndCheckHandle(unshared);
> default -> throw new StreamCorruptedException(String.format("invalid
> type code: %02X", tc));
> };
> return descriptor;
> }
>
> , where the body of TC_REFERENCE is enclosed in readAndCheckHandle, OR
>
> 2) Simply
>
> case TC_REFERENCE -> {
> var d = (ObjectStreamClass)readHandle(unshared);
> // Should only reference initialized class descriptors
> d.checkInitialized();
> yield d; }
Code updated as suggested. See adc8af4
> src/java.base/share/classes/java/io/StreamTokenizer.java line 795:
>
>> 793: * case statements
>> 794: */
>> 795: if (ttype < 256 && ((ctype[ttype] & CT_QUOTE) != 0)) {
>
> Maybe (since its easier to grok the yield rather than the assignment of ret
> in branches):
>
> String ret = switch (ttype) {
> case TT_EOF -> "EOF";
> case TT_EOL -> "EOL";
> case TT_WORD-> sval;
> case TT_NUMBER -> "n=" + nval;
> case TT_NOTHING -> "NOTHING";
> default -> {
> /*
> * ttype is the first character of either a quoted string or
> * is an ordinary character. ttype can definitely not be less
> * than 0, since those are reserved values used in the
> previous
> * case statements
> */
> if (ttype < 256 && ((ctype[ttype] & CT_QUOTE) != 0)) {
> yield sval;
> }
> char s[] = new char[3];
> s[0] = s[2] = ''';
> s[1] = (char) ttype;
> yield new String(s);
> }
> };
> return "Token[" + ret + "], line " + LINENO;
Code updated as suggested. See adc8af4
-
PR: https://git.openjdk.java.net/jdk/pull/4182
Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v2]
On Tue, 25 May 2021 13:16:00 GMT, Daniel Fuchs 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 three additional
>> commits since the last revision:
>>
>> - 8267670: Updated code to use yield
>> - Merge remote-tracking branch 'origin/master' into JDK-8267670
>> - 8267670: Update java.io, java.math, and java.text to use switch
>> expressions
>
> src/java.base/share/classes/java/io/ObjectStreamField.java line 123:
>
>> 121: case 'D' -> type = Double.TYPE;
>> 122: case 'L', '[' -> type = Object.class;
>> 123: default -> throw new
>> IllegalArgumentException("illegal signature");
>
> Why not assign type here?
>
>
> type = switch(signature.charAt(0)) {
> case 'Z' -> Boolean.TYPE;
>
Thanks for your suggestion. I've done that now. See adc8af4
-
PR: https://git.openjdk.java.net/jdk/pull/4182
Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v2]
> Hi, > > Could someone please review my code for updating the code in the `java.io`, > `java.math`, and `java.text` 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 three additional commits since the last revision: - 8267670: Updated code to use yield - Merge remote-tracking branch 'origin/master' into JDK-8267670 - 8267670: Update java.io, java.math, and java.text to use switch expressions - Changes: - all: https://git.openjdk.java.net/jdk/pull/4182/files - new: https://git.openjdk.java.net/jdk/pull/4182/files/b98e47db..adc8af41 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4182=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4182=00-01 Stats: 143 lines in 16 files changed: 28 ins; 56 del; 59 mod Patch: https://git.openjdk.java.net/jdk/pull/4182.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4182/head:pull/4182 PR: https://git.openjdk.java.net/jdk/pull/4182
