Re: RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v4]
On Tue, 3 Aug 2021 23:42:55 GMT, Sergey Bylokhov wrote:
>> This is a request to clean up a desktop module as was done in JDK-8233884
>> for "java.base" module.
>>
>> In many places standard charsets are looked up via their names, for example:
>> absolutePath.getBytes("UTF-8");
>>
>> This could be done more efficiently(x20 time faster) with use of
>> java.nio.charset.StandardCharsets:
>> absolutePath.getBytes(StandardCharsets.UTF_8);
>>
>> The later variant also makes the code cleaner, as it is known not to throw
>> UnsupportedEncodingException in contrary to the former variant.
>>
>> Tested by the desktop headless/headful tests on linux/windows.
>
> Sergey Bylokhov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Update IPPPrintService.java
I admit I prefer static imports in this case: the code is shorter and is as
clear. It's pretty obvious where the encoding comes from.
-
Marked as reviewed by aivanov (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4951
Re: RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v4]
> This is a request to clean up a desktop module as was done in JDK-8233884 for
> "java.base" module.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> Tested by the desktop headless/headful tests on linux/windows.
Sergey Bylokhov has updated the pull request incrementally with one additional
commit since the last revision:
Update IPPPrintService.java
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4951/files
- new: https://git.openjdk.java.net/jdk/pull/4951/files/e4e82c83..47ed1e81
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4951=03
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4951=02-03
Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod
Patch: https://git.openjdk.java.net/jdk/pull/4951.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4951/head:pull/4951
PR: https://git.openjdk.java.net/jdk/pull/4951
Re: RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v2]
On Tue, 3 Aug 2021 22:42:55 GMT, Alexey Ivanov wrote: >> I am fine to do that, if there are no objections I can change the whole fix. > >> >> I am fine to do that, if there are no objections I can change the whole fix. > > Modifying the entire changeset seems like an overkill. > Using static imports in only one file is _inconsistent_, yet it makes the > places where the encodings are used clearer. Updated - PR: https://git.openjdk.java.net/jdk/pull/4951
Re: RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v3]
> This is a request to clean up a desktop module as was done in JDK-8233884 for
> "java.base" module.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> Tested by the desktop headless/headful tests on linux/windows.
Sergey Bylokhov has updated the pull request incrementally with one additional
commit since the last revision:
static import StandardCharsets
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4951/files
- new: https://git.openjdk.java.net/jdk/pull/4951/files/886264aa..e4e82c83
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4951=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4951=01-02
Stats: 245 lines in 31 files changed: 93 ins; 63 del; 89 mod
Patch: https://git.openjdk.java.net/jdk/pull/4951.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4951/head:pull/4951
PR: https://git.openjdk.java.net/jdk/pull/4951
Re: RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v2]
On Tue, 3 Aug 2021 22:03:54 GMT, Sergey Bylokhov wrote: > > I am fine to do that, if there are no objections I can change the whole fix. Modifying the entire changeset seems like an overkill. Using static imports in only one file is _inconsistent_, yet it makes the places where the encodings are used clearer. - PR: https://git.openjdk.java.net/jdk/pull/4951
Re: RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v2]
On Tue, 3 Aug 2021 21:54:08 GMT, Alexey Ivanov wrote: >> it is aligned already, the StandardCharsets.UTF_8 is parameter of "new >> String()", not the getTransferData. > > Ah, right! > But it's confusing: it looks as if `StandardCharsets.UTF_8` is a parameter to > `getTransferData`. Maybe avoid breaking the line and leave `UTF_8` on the > same line? If you import `UTF_8` and `UTF_16LE` statically, line break is > unnecessary. I am fine to do that, if there are no objections I can change the whole fix. - PR: https://git.openjdk.java.net/jdk/pull/4951
Re: RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v2]
On Tue, 3 Aug 2021 21:39:14 GMT, Sergey Bylokhov wrote: >> src/java.desktop/windows/classes/sun/awt/windows/WDataTransferer.java line >> 270: >> >>> 268: charset = new String((byte[])localeTransferable. >>> 269: getTransferData(javaTextEncodingFlavor), >>> 270: StandardCharsets.UTF_8); >> >> Suggestion: >> >> getTransferData(javaTextEncodingFlavor), >> StandardCharsets.UTF_8); >> >> The parameter on the second line should probably be aligned with the first >> parameter as it's done in the snippet above. > > it is aligned already, the StandardCharsets.UTF_8 is parameter of "new > String()", not the getTransferData. Ah, right! But it's confusing: it looks as if `StandardCharsets.UTF_8` is a parameter to `getTransferData`. Maybe avoid breaking the line and leave `UTF_8` on the same line? If you import `UTF_8` and `UTF_16LE` statically, line break is unnecessary. - PR: https://git.openjdk.java.net/jdk/pull/4951
Re: RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v2]
On Tue, 3 Aug 2021 21:18:40 GMT, Alexey Ivanov wrote: >> Sergey Bylokhov 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 branch 'master' into JDK-8271456 >> - Initial fix for JDK-8271456 > > src/java.desktop/windows/classes/sun/awt/windows/WDataTransferer.java line > 270: > >> 268: charset = new String((byte[])localeTransferable. >> 269: getTransferData(javaTextEncodingFlavor), >> 270: StandardCharsets.UTF_8); > > Suggestion: > > getTransferData(javaTextEncodingFlavor), > StandardCharsets.UTF_8); > > The parameter on the second line should probably be aligned with the first > parameter as it's done in the snippet above. it is aligned already, the StandardCharsets.UTF_8 is parameter of "new String()", not the getTransferData. - PR: https://git.openjdk.java.net/jdk/pull/4951
Re: RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v2]
On Tue, 3 Aug 2021 19:37:04 GMT, Sergey Bylokhov wrote:
>> This is a request to clean up a desktop module as was done in JDK-8233884
>> for "java.base" module.
>>
>> In many places standard charsets are looked up via their names, for example:
>> absolutePath.getBytes("UTF-8");
>>
>> This could be done more efficiently(x20 time faster) with use of
>> java.nio.charset.StandardCharsets:
>> absolutePath.getBytes(StandardCharsets.UTF_8);
>>
>> The later variant also makes the code cleaner, as it is known not to throw
>> UnsupportedEncodingException in contrary to the former variant.
>>
>> Tested by the desktop headless/headful tests on linux/windows.
>
> Sergey Bylokhov 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 branch 'master' into JDK-8271456
> - Initial fix for JDK-8271456
Marked as reviewed by aivanov (Reviewer).
src/java.desktop/windows/classes/sun/awt/windows/WDataTransferer.java line 270:
> 268: charset = new String((byte[])localeTransferable.
> 269: getTransferData(javaTextEncodingFlavor),
> 270: StandardCharsets.UTF_8);
Suggestion:
getTransferData(javaTextEncodingFlavor),
StandardCharsets.UTF_8);
The parameter on the second line should probably be aligned with the first
parameter as it's done in the snippet above.
-
PR: https://git.openjdk.java.net/jdk/pull/4951
Re: RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v2]
> This is a request to clean up a desktop module as was done in JDK-8233884 for
> "java.base" module.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> Tested by the desktop headless/headful tests on linux/windows.
Sergey Bylokhov 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 branch 'master' into JDK-8271456
- Initial fix for JDK-8271456
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4951/files
- new: https://git.openjdk.java.net/jdk/pull/4951/files/430c9b3a..886264aa
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4951=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4951=00-01
Stats: 1774 lines in 83 files changed: 1230 ins; 337 del; 207 mod
Patch: https://git.openjdk.java.net/jdk/pull/4951.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4951/head:pull/4951
PR: https://git.openjdk.java.net/jdk/pull/4951
Re: RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module
On Sun, 1 Aug 2021 07:07:21 GMT, Sergey Bylokhov wrote:
> This is a request to clean up a desktop module as was done in JDK-8233884 for
> "java.base" module.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> Tested by the desktop headless/headful tests on linux/windows.
Marked as reviewed by azvegint (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/4951
Re: RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module
On Tue, 3 Aug 2021 13:30:28 GMT, Alexander Zvegintsev
wrote:
>> This is a request to clean up a desktop module as was done in JDK-8233884
>> for "java.base" module.
>>
>> In many places standard charsets are looked up via their names, for example:
>> absolutePath.getBytes("UTF-8");
>>
>> This could be done more efficiently(x20 time faster) with use of
>> java.nio.charset.StandardCharsets:
>> absolutePath.getBytes(StandardCharsets.UTF_8);
>>
>> The later variant also makes the code cleaner, as it is known not to throw
>> UnsupportedEncodingException in contrary to the former variant.
>>
>> Tested by the desktop headless/headful tests on linux/windows.
>
> src/java.desktop/share/classes/com/sun/imageio/plugins/wbmp/WBMPMetadata.java
> line 25:
>
>> 23: * questions.
>> 24: */
>> 25:
>
> Since you updating a copyright year in all other files, you probably might
> want to update in this file too.
I did not update it since this is the only file where the imports are updated
only(the UnsupportedEncodingException is removed).
-
PR: https://git.openjdk.java.net/jdk/pull/4951
Re: RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module
On Sun, 1 Aug 2021 07:07:21 GMT, Sergey Bylokhov wrote:
> This is a request to clean up a desktop module as was done in JDK-8233884 for
> "java.base" module.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> Tested by the desktop headless/headful tests on linux/windows.
src/java.desktop/share/classes/com/sun/imageio/plugins/wbmp/WBMPMetadata.java
line 25:
> 23: * questions.
> 24: */
> 25:
Since you updating a copyright year in all other files, you probably might want
to update in this file too.
-
PR: https://git.openjdk.java.net/jdk/pull/4951
Re: RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module
On Sun, 1 Aug 2021 07:07:21 GMT, Sergey Bylokhov wrote:
> This is a request to clean up a desktop module as was done in JDK-8233884 for
> "java.base" module.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> Tested by the desktop headless/headful tests on linux/windows.
Marked as reviewed by jdv (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/4951
Re: RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module
On Sun, 1 Aug 2021 07:07:21 GMT, Sergey Bylokhov wrote:
> This is a request to clean up a desktop module as was done in JDK-8233884 for
> "java.base" module.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> Tested by the desktop headless/headful tests on linux/windows.
Marked as reviewed by stsypa...@github.com (no known OpenJDK username).
-
PR: https://git.openjdk.java.net/jdk/pull/4951
RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module
This is a request to clean up a desktop module as was done in JDK-8233884 for
"java.base" module.
In many places standard charsets are looked up via their names, for example:
absolutePath.getBytes("UTF-8");
This could be done more efficiently(x20 time faster) with use of
java.nio.charset.StandardCharsets:
absolutePath.getBytes(StandardCharsets.UTF_8);
The later variant also makes the code cleaner, as it is known not to throw
UnsupportedEncodingException in contrary to the former variant.
Tested by the desktop headless/headful tests on linux/windows.
-
Commit messages:
- Initial fix for JDK-8271456
Changes: https://git.openjdk.java.net/jdk/pull/4951/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4951=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8271456
Stats: 641 lines in 28 files changed: 276 ins; 237 del; 128 mod
Patch: https://git.openjdk.java.net/jdk/pull/4951.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4951/head:pull/4951
PR: https://git.openjdk.java.net/jdk/pull/4951
