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 [v3]
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 [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
Re: RFR: 8270490: Charset.forName() taking fallback default value [v3]
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]
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]
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]
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]
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]
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 [v2]
On Fri, 22 Oct 2021 04:54:35 GMT, Glavo wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Moved the null sentence into @param tag.
>
> Oh, I found that I checked the outdated source code. Now this problem does
> not exist in `StringCoding`.
>
> I simply browsed the latest source code of JDK and found that this idiom no
> longer appears outside jline. I believe that the source code of jline does
> not need to be modified in openjdk.
>
> But I noticed `LauncherHelper.makePlatformString`
> (https://github.com/openjdk/jdk/blob/c978ca87de2d9152345dfd85983278c42bb28cd3/src/java.base/share/classes/sun/launcher/LauncherHelper.java#L887)
>
> I don't understand why it stores `encoding` string and `isCharsetSupported`
> boolean values. Nor do I find references to these two fields in native code.
> I think it may be improved in this PR like this:
>
>
>
> private static final String encprop = "sun.jnu.encoding";
> private static Charset charset = null;
>
> /*
> * converts a c or a byte array to a platform specific string,
> * previously implemented as a native method in the launcher.
> */
> static String makePlatformString(boolean printToStderr, byte[] inArray) {
> initOutput(printToStderr);
> if (charset == null) {
> charset = Charset.forName(System.getProperty(encprop),
> Charset.defaultCharset());
> }
> return new String(inArray, charset);
> }
@Glavo, thank you for the suggestions. Although applying the new method in the
JDK code would be useful, I'd keep this PR just to introduce this new method.
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Fri, 22 Oct 2021 02:07:44 GMT, Sergey Bylokhov wrote:
>> I first thought of swallowing all exceptions in 2-arg forName(), but decided
>> not to do that. Because `IllegalArgumentException` and
>> `IllegalCharsetNameException` are for the validity of the passed
>> `charsetName`, like detecting `null` or invalid chars like "". On the other
>> hand, `UnsupportedCharsetException` is for the availability which varies
>> depending on the user's settings and or platform, which can be safely
>> replaced with `fallback` charset. So yes, it is not totally getting rid of
>> `try-catch` but it avoids `UnsupportedCharsetException` which is only
>> detectable at runtime.
>
> Then what is the benefit, if the user will have to write such code anyway?:
>
> try {
> cs = Charset.forName(StaticProperty.nativeEncoding(),
> fallback);
> } catch (Exception ignored) {
> cs = fallback;
> }
>
> Even in the current code update it can work well w/o the second parameter.
OK, I revised the API to swallow `IllegalCharsetNameException`. This will
effectively remove try-catch clauses, as `IllegalArgumentException` is
considered an error, and simply avoided by a null check.
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v3]
> 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
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Wed, 20 Oct 2021 19:02:30 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:
>
> Moved the null sentence into @param tag.
Oh, I found that I checked the outdated source code. Now this problem does not
exist in `StringCoding`.
I simply browsed the latest source code of JDK and found that this idiom no
longer appears outside jline. I believe that the source code of jline does not
need to be modified in openjdk.
But I noticed `LauncherHelper.makePlatformString`
(https://github.com/openjdk/jdk/blob/c978ca87de2d9152345dfd85983278c42bb28cd3/src/java.base/share/classes/sun/launcher/LauncherHelper.java#L887)
I don't understand why it stores `encoding` string and `isCharsetSupported`
boolean values. Nor do I find references to these two fields in native code. I
think it can benefit from the improvement in this PR like this?
private static final String encprop = "sun.jnu.encoding";
private static Charset charset = null;
/*
* converts a c or a byte array to a platform specific string,
* previously implemented as a native method in the launcher.
*/
static String makePlatformString(boolean printToStderr, byte[] inArray) {
initOutput(printToStderr);
if (charset == null) {
charset = Charset.forName(encprop, Charset.defaultCharset());
}
return new String(inArray, charset);
}
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Wed, 20 Oct 2021 19:02:30 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:
>
> Moved the null sentence into @param tag.
I'm not reviewer.
I think maybe we should check all the usage of `Charset.isSupported`.
At present, `Charset.isSupported` and `Charset.forName` are often used
continuously, such as in `StringCoding`:
class StringCoding {
// ...
private static Charset lookupCharset(String csn) {
if (Charset.isSupported(csn)) {
try {
return Charset.forName(csn);
} catch (UnsupportedCharsetException x) {
throw new Error(x);
}
}
return null;
}
//...
}
This calls `Charset.lookup` twice. Replacing it with such code should eliminate
unnecessary lookup:
private static Charset lookupCharset(String csn) {
return Charset.forName(csn, null);
}
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Thu, 21 Oct 2021 16:03:29 GMT, Naoto Sato wrote:
>> Apparently `IllegalCharsetNameException` or `IllegalArgumentException` could
>> still be thrown - so removing the `try-catch` would be a change of behaviour
>> in those cases. It all depends on whether there is a chance that these
>> exceptions could be thrown in this particular context (with these particular
>> input parameters) - which I am not able to tell - but maybe someone more
>> familiar with this code could...
>
> I first thought of swallowing all exceptions in 2-arg forName(), but decided
> not to do that. Because `IllegalArgumentException` and
> `IllegalCharsetNameException` are for the validity of the passed
> `charsetName`, like detecting `null` or invalid chars like "". On the other
> hand, `UnsupportedCharsetException` is for the availability which varies
> depending on the user's settings and or platform, which can be safely
> replaced with `fallback` charset. So yes, it is not totally getting rid of
> `try-catch` but it avoids `UnsupportedCharsetException` which is only
> detectable at runtime.
Then what is the benefit, if the user will have to write such code anyway?:
try {
cs = Charset.forName(StaticProperty.nativeEncoding(), fallback);
} catch (Exception ignored) {
cs = fallback;
}
Even in the current code update it can work well w/o the second parameter.
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Thu, 21 Oct 2021 18:00:46 GMT, Naoto Sato wrote:
>> I'm not reviewer.
>>
>> I'd like to write down following code without `try-catch`.
>>
>> var cs = Charset.forName(charsetName, null);
>> if (cs == null) cs = StandardCharsets.UTF_8;
>>
>> or please find out more easy way.
>
>> I'd like to write down following code without `try-catch`.
>
> You don't *have to* try-catch those exceptions if you are not interested, as
> they are subclasses of `RuntimeException`.
>
>> ```
>> var cs = Charset.forName(charsetName, null);
>> if (cs == null) cs = StandardCharsets.UTF_8;
>> ```
>
> This could be simplified to
>
>
> var cs = Charset.forName(charsetName, StandardCharsets.UTF_8);
@naotoj
Oh sorry, I'd like to detect fallback charset is used, like:
var cs = Charset.forName(charsetName, null);
if (cs == null) {
System.err.println("Used UTF-8 encoding instead of "+charsetName+");
cs = StandardCharsets.UTF_8;
}
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Thu, 21 Oct 2021 16:06:31 GMT, Ichiroh Takiguchi wrote: > I'd like to write down following code without `try-catch`. You don't *have to* try-catch those exceptions if you are not interested, as they are subclasses of `RuntimeException`. > ``` > var cs = Charset.forName(charsetName, null); > if (cs == null) cs = StandardCharsets.UTF_8; > ``` This could be simplified to var cs = Charset.forName(charsetName, StandardCharsets.UTF_8); - PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Wed, 20 Oct 2021 19:02:30 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: > > Moved the null sentence into @param tag. I'm not reviewer. I'd like to write down following code without `try-catch`. var cs = Charset.forName(charsetName, null); if (cs == null) cs = StandardCharsets.UTF_8; or please find out more easy way. - PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Thu, 21 Oct 2021 09:33:30 GMT, Daniel Fuchs wrote: >> Right, I think both try-catch usages will be removed. > > Apparently `IllegalCharsetNameException` or `IllegalArgumentException` could > still be thrown - so removing the `try-catch` would be a change of behaviour > in those cases. It all depends on whether there is a chance that these > exceptions could be thrown in this particular context (with these particular > input parameters) - which I am not able to tell - but maybe someone more > familiar with this code could... I first thought of swallowing all exceptions in 2-arg forName(), but decided not to do that. Because `IllegalArgumentException` and `IllegalCharsetNameException` are for the validity of the passed `charsetName`, like detecting `null` or invalid chars like "". On the other hand, `UnsupportedCharsetException` is for the availability which varies depending on the user's settings and or platform, which can be safely replaced with `fallback` charset. So yes, it is not totally getting rid of `try-catch` but it avoids `UnsupportedCharsetException` which is only detectable at runtime. - PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Thu, 21 Oct 2021 15:00:03 GMT, Roger Riggs wrote:
>> src/java.base/share/classes/java/io/Console.java line 594:
>>
>>> 592: cs = Charset.forName(StaticProperty.nativeEncoding(),
>>> Charset.defaultCharset());
>>> 593: } catch (Exception ignored) {
>>> 594: cs = Charset.defaultCharset();
>>
>> What kind of actual improvements do we get here since the catch block is
>> still in place?
>
> In the case of Console, both charset names come from system properties and
> could refer to invalid or unavailable charsets. (null is handled
> separately). The code silently ignores the invalid values. The new method ,
> as is, is not a fully satisfying replacement. Catching Exception is too
> broad a catch but may be warranted in this case so that some Console charset
> is selected.
>
> The new method would be useful in more cases if the default was returned for
> any of
> IllegalCharsetNameException, IllegalArgumentException, and
> UnsupportedCharsetException.
Since we do support all the encodings for platforms we support out-of-the-box,
it could still be possible that the user can specify his/her console encoding
to the one we do not support. In that case, we can safely use the default
`UTF-8` without throwing/catching `UnsupportedCharsetException`.
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Thu, 21 Oct 2021 01:31:31 GMT, Sergey Bylokhov wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Moved the null sentence into @param tag.
>
> src/java.base/share/classes/java/io/Console.java line 594:
>
>> 592: cs = Charset.forName(StaticProperty.nativeEncoding(),
>> Charset.defaultCharset());
>> 593: } catch (Exception ignored) {
>> 594: cs = Charset.defaultCharset();
>
> What kind of actual improvements do we get here since the catch block is
> still in place?
In the case of Console, both charset names come from system properties and
could refer to invalid or unavailable charsets. (null is handled separately).
The code silently ignores the invalid values. The new method , as is, is not a
fully satisfying replacement. Catching Exception is too broad a catch but may
be warranted in this case so that some Console charset is selected.
The new method would be useful in more cases if the default was returned for
any of
IllegalCharsetNameException, IllegalArgumentException, and
UnsupportedCharsetException.
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Thu, 21 Oct 2021 06:50:41 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/java/io/Console.java line 587:
>>
>>> 585: try {
>>> 586: cs = Charset.forName(csname, null);
>>> 587: } catch (Exception ignored) { }
>>
>> The comment which suggests this enhancement was about eliminating such
>> "exception ignored" code paths. Is it still needed here? And if it is needed
>> why do we pass the null as a fallback?
>
> Right, I think both try-catch usages will be removed.
Apparently `IllegalCharsetNameException` or `IllegalArgumentException` could
still be thrown - so removing the `try-catch` would be a change of behaviour in
those cases. It all depends on whether there is a chance that these exceptions
could be thrown in this particular context (with these particular input
parameters) - which I am not able to tell - but maybe someone more familiar
with this code could...
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Thu, 21 Oct 2021 01:30:05 GMT, Sergey Bylokhov wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Moved the null sentence into @param tag.
>
> src/java.base/share/classes/java/io/Console.java line 587:
>
>> 585: try {
>> 586: cs = Charset.forName(csname, null);
>> 587: } catch (Exception ignored) { }
>
> The comment which suggests this enhancement was about eliminating such
> "exception ignored" code paths. Is it still needed here? And if it is needed
> why do we pass the null as a fallback?
Right, I think both try-catch usages will be removed.
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Wed, 20 Oct 2021 19:02:30 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:
>
> Moved the null sentence into @param tag.
src/java.base/share/classes/java/io/Console.java line 587:
> 585: try {
> 586: cs = Charset.forName(csname, null);
> 587: } catch (Exception ignored) { }
The comment which suggests this enhancement was about eliminating such
"exception ignored" code paths. Is it still needed here? And if it is needed
why do we pass the null as a fallback?
src/java.base/share/classes/java/io/Console.java line 594:
> 592: cs = Charset.forName(StaticProperty.nativeEncoding(),
> Charset.defaultCharset());
> 593: } catch (Exception ignored) {
> 594: cs = Charset.defaultCharset();
What kind of actual improvements do we get here since the catch block is still
in place?
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Wed, 20 Oct 2021 19:02:30 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: > > Moved the null sentence into @param tag. Looks good - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Wed, 20 Oct 2021 19:02:30 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: > > Moved the null sentence into @param tag. Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Wed, 20 Oct 2021 18:58:34 GMT, Alan Bateman wrote: >> Thanks, Joe. Moved that explanation into the `fallback` param, which I >> initially intended. > > The java.nio.charset package has the usual "Unless otherwise noted, passing a > null argument ..." so if fallback is allowed to be null then you will need to > add ", can null" to its description. Our comments crossed I guess? Moved the null allowance into `fallback`. - PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Wed, 20 Oct 2021 18:37:05 GMT, Joe Wang wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Moved the null sentence into @param tag.
>
> src/java.base/share/classes/java/nio/charset/Charset.java line 545:
>
>> 543: * @return A charset object for the named charset, or {@code
>> fallback}
>> 544: * in case the charset object for the named charset is not
>> 545: * available. May be {@code null}
>
> A minor comment: it seems to me returning the fallback charset is sufficient,
> and "May be null" may be not necessary since the fallback charset is
> provided, it's expected if it's null.
Thanks, Joe. Moved that explanation into the `fallback` param, which I
initially intended.
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
On Wed, 20 Oct 2021 18:56:22 GMT, Naoto Sato wrote:
>> src/java.base/share/classes/java/nio/charset/Charset.java line 545:
>>
>>> 543: * @return A charset object for the named charset, or {@code
>>> fallback}
>>> 544: * in case the charset object for the named charset is not
>>> 545: * available. May be {@code null}
>>
>> A minor comment: it seems to me returning the fallback charset is
>> sufficient, and "May be null" may be not necessary since the fallback
>> charset is provided, it's expected if it's null.
>
> Thanks, Joe. Moved that explanation into the `fallback` param, which I
> initially intended.
The java.nio.charset package has the usual "Unless otherwise noted, passing a
null argument ..." so if fallback is allowed to be null then you will need to
add ", can null" to its description.
-
PR: https://git.openjdk.java.net/jdk/pull/6045
Re: RFR: 8270490: Charset.forName() taking fallback default value [v2]
> 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: Moved the null sentence into @param tag. - Changes: - all: https://git.openjdk.java.net/jdk/pull/6045/files - new: https://git.openjdk.java.net/jdk/pull/6045/files/a31dbdc7..4c2142be Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6045=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6045=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 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
Re: RFR: 8270490: Charset.forName() taking fallback default value
On Wed, 20 Oct 2021 17:23:36 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
Marked as reviewed by joehw (Reviewer).
src/java.base/share/classes/java/nio/charset/Charset.java line 545:
> 543: * @return A charset object for the named charset, or {@code
> fallback}
> 544: * in case the charset object for the named charset is not
> 545: * available. May be {@code null}
A minor comment: it seems to me returning the fallback charset is sufficient,
and "May be null" may be not necessary since the fallback charset is provided,
it's expected if it's null.
-
PR: https://git.openjdk.java.net/jdk/pull/6045
RFR: 8270490: Charset.forName() taking fallback default value
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 - Commit messages: - 8270490: Charset.forName() taking fallback default value Changes: https://git.openjdk.java.net/jdk/pull/6045/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6045=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8270490 Stats: 117 lines in 3 files changed: 115 ins; 0 del; 2 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
