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