Re: RFR: 8270490: Charset.forName() taking fallback default value [v3]

[Naoto Sato]

On Sat, 23 Oct 2021 19:29:30 GMT, Sergey Bylokhov  wrote:

> Just an idea, should we check that the second parameter is null or not? Any 
> pros and cons of that? For example should this code be allowed:
> 
> ```
> var cs = Charset.forName(charsetName, null);
> if (cs == null) {
>System.err.println("Used UTF-8 encoding instead of "+charsetName+");
>cs = StandardCharsets.UTF_8;
> }
> ```

Yes, that's the whole purpose of allowing `null` for `fallback`.

> src/java.base/share/classes/java/io/Console.java line 589:
> 
>> 587: }
>> 588: if (cs == null) {
>> 589: cs = Charset.forName(StaticProperty.nativeEncoding(), 
>> Charset.defaultCharset());
> 
> Not sure but looks like this class tries to maintain 80 chars per line rule?

Fixed.

-

PR: https://git.openjdk.java.net/jdk/pull/6045

Re: RFR: 8270490: Charset.forName() taking fallback default value [v3]

[Naoto Sato]

On Sat, 23 Oct 2021 06:42:52 GMT, Alan Bateman  wrote:

> I think you'll need to adjust the description make it clear that "fallback" 
> is used when the input is not a legal charset name or the charset is not 
> available.

Made the method description clearer.

-

PR: https://git.openjdk.java.net/jdk/pull/6045

Re: RFR: 8270490: Charset.forName() taking fallback default value [v3]

[Sergey Bylokhov]

On Fri, 22 Oct 2021 17:33:43 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:
> 
>   Removed `@throws IllegalCharsetNameException`

Just an idea, should we check that the second parameter is null or not? Any 
pros and cons of that?
For example should this code be allowed:

var cs = Charset.forName(charsetName, null);
if (cs == null) {
   System.err.println("Used UTF-8 encoding instead of "+charsetName+");
   cs = StandardCharsets.UTF_8;
}

Or something like this should be forced:

var cs = Charset.forName(charsetName, fallback);
if (cs == fallback) {
   System.err.println("Used UTF-8 encoding instead of "+charsetName+");
}

I have no preference.

-

PR: https://git.openjdk.java.net/jdk/pull/6045

Re: RFR: 8270490: Charset.forName() taking fallback default value [v3]

[Alan Bateman]

On Fri, 22 Oct 2021 17:33:43 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:
> 
>   Removed `@throws IllegalCharsetNameException`

I think you'll need to adjust the description make it clear that "fallback" is 
used when the input is not a legal charset name or the charset is not available.

-

PR: https://git.openjdk.java.net/jdk/pull/6045

Re: RFR: 8270490: Charset.forName() taking fallback default value [v3]

[Ichiroh Takiguchi]

On Fri, 22 Oct 2021 17:33:43 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:
> 
>   Removed `@throws IllegalCharsetNameException`

About javadoc, I think following description is not clear.

 * @param  fallback
 * fallback charset in case the charset object for the named
 * charset is not available. May be {@code null}

-

PR: https://git.openjdk.java.net/jdk/pull/6045

Re: RFR: 8270490: Charset.forName() taking fallback default value [v3]

[Sergey Bylokhov]

On Fri, 22 Oct 2021 17:33:43 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:
> 
>   Removed `@throws IllegalCharsetNameException`

src/java.base/share/classes/java/io/Console.java line 589:

> 587: }
> 588: if (cs == null) {
> 589: cs = Charset.forName(StaticProperty.nativeEncoding(), 
> Charset.defaultCharset());

Not sure but looks like this class tries to maintain 80 chars per line rule?

-

PR: https://git.openjdk.java.net/jdk/pull/6045

Re: RFR: 8270490: Charset.forName() taking fallback default value [v3]

[Roger Riggs]

On Fri, 22 Oct 2021 17:33:43 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:
> 
>   Removed `@throws IllegalCharsetNameException`

Marked as reviewed by rriggs (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/6045

Re: RFR: 8270490: Charset.forName() taking fallback default value [v3]

[Naoto Sato]

> 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:

  Removed `@throws IllegalCharsetNameException`

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6045/files
  - new: https://git.openjdk.java.net/jdk/pull/6045/files/4c2142be..7c73c5ba

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=6045=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6045=01-02

  Stats: 32 lines in 3 files changed: 6 ins; 22 del; 4 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