Re: RFR: 8270490: Charset.forName() taking fallback default value [v4]
On Sat, 23 Oct 2021 22:13:35 GMT, Naoto Sato wrote: >> During the review of JEP 400, a proposal to provide an overloaded method to >> `Charset.forName()` was suggested >> [[1]](https://github.com/openjdk/jdk/pull/4733#discussion_r669693954). This >> PR is to implement the proposal. A CSR is also drafted as >> https://bugs.openjdk.java.net/browse/JDK-8275348 > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reflecting review comments Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v4]
On Tue, 26 Oct 2021 12:59:11 GMT, Naoto Sato wrote:
>> src/java.base/share/classes/java/io/Console.java line 590:
>>
>>> 588: if (cs == null) {
>>> 589: cs = Charset.forName(StaticProperty.nativeEncoding(),
>>> 590: Charset.defaultCharset());
>>
>> I assume that `StaticProperty.nativeEncoding()` will never be `null`?
>> Otherwise an IAE would be thrown here where previously
>> `Charset.defaultCharset()` would be used.
>
> Yes. `StaticProperty.nativeEncoding()` caches the value to `native.encoding`
> system property. The value is not optional, so it should be considered an
> error if `StaticProperty.nativeEncoding()` returned `null`.
> https://download.java.net/java/early_access/jdk18/docs/api/java.base/java/lang/System.html#native.encoding
Thanks for the clarification.
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v4]
On Tue, 26 Oct 2021 10:42:49 GMT, Daniel Fuchs wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Reflecting review comments
>
> src/java.base/share/classes/java/io/Console.java line 590:
>
>> 588: if (cs == null) {
>> 589: cs = Charset.forName(StaticProperty.nativeEncoding(),
>> 590: Charset.defaultCharset());
>
> I assume that `StaticProperty.nativeEncoding()` will never be `null`?
> Otherwise an IAE would be thrown here where previously
> `Charset.defaultCharset()` would be used.
Yes. `StaticProperty.nativeEncoding()` caches the value to `native.encoding`
system property. The value is not optional, so it should be considered an error
if `StaticProperty.nativeEncoding()` returned `null`.
https://download.java.net/java/early_access/jdk18/docs/api/java.base/java/lang/System.html#native.encoding
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v4]
On Sat, 23 Oct 2021 22:13:35 GMT, Naoto Sato wrote:
>> During the review of JEP 400, a proposal to provide an overloaded method to
>> `Charset.forName()` was suggested
>> [[1]](https://github.com/openjdk/jdk/pull/4733#discussion_r669693954). This
>> PR is to implement the proposal. A CSR is also drafted as
>> https://bugs.openjdk.java.net/browse/JDK-8275348
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reflecting review comments
src/java.base/share/classes/java/io/Console.java line 590:
> 588: if (cs == null) {
> 589: cs = Charset.forName(StaticProperty.nativeEncoding(),
> 590: Charset.defaultCharset());
I assume that `StaticProperty.nativeEncoding()` will never be `null`? Otherwise
an IAE would be thrown here where previously `Charset.defaultCharset()` would
be used.
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v4]
On Sat, 23 Oct 2021 22:13:35 GMT, Naoto Sato wrote: >> During the review of JEP 400, a proposal to provide an overloaded method to >> `Charset.forName()` was suggested >> [[1]](https://github.com/openjdk/jdk/pull/4733#discussion_r669693954). This >> PR is to implement the proposal. A CSR is also drafted as >> https://bugs.openjdk.java.net/browse/JDK-8275348 > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reflecting review comments Thank you for clarification. - Marked as reviewed by serb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v4]
On Sat, 23 Oct 2021 22:13:35 GMT, Naoto Sato wrote: >> During the review of JEP 400, a proposal to provide an overloaded method to >> `Charset.forName()` was suggested >> [[1]](https://github.com/openjdk/jdk/pull/4733#discussion_r669693954). This >> PR is to implement the proposal. A CSR is also drafted as >> https://bugs.openjdk.java.net/browse/JDK-8275348 > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reflecting review comments Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v4]
On Sat, 23 Oct 2021 22:13:35 GMT, Naoto Sato wrote: >> During the review of JEP 400, a proposal to provide an overloaded method to >> `Charset.forName()` was suggested >> [[1]](https://github.com/openjdk/jdk/pull/4733#discussion_r669693954). This >> PR is to implement the proposal. A CSR is also drafted as >> https://bugs.openjdk.java.net/browse/JDK-8275348 > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reflecting review comments Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v4]
> During the review of JEP 400, a proposal to provide an overloaded method to > `Charset.forName()` was suggested > [[1]](https://github.com/openjdk/jdk/pull/4733#discussion_r669693954). This > PR is to implement the proposal. A CSR is also drafted as > https://bugs.openjdk.java.net/browse/JDK-8275348 Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Reflecting review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/6045/files - new: https://git.openjdk.java.net/jdk/pull/6045/files/7c73c5ba..0e787e7a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6045=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6045=02-03 Stats: 7 lines in 2 files changed: 2 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/6045.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6045/head:pull/6045 PR: https://git.openjdk.java.net/jdk/pull/6045
