On Sun, 17 Jan 2021 09:03:30 GMT, Peter Levart <plev...@openjdk.org> wrote:
>>> Do you think this code belongs more to String than to StringCoding?
>>
>> I consider StringCoding an implementation detail of String, so I'm not sure
>> there's (much) value in maintaining the separation of concern if it is cause
>> for a performance loss. While encapsulating and separating concerns is a
>> fine purpose I think the main purpose served by StringCoding is to resolve
>> some bootstrap issues: String is one of the first classes to be loaded and
>> eagerly pulling in dependencies like ThreadLocal and Charsets before
>> bootstrapping leads to all manner of hard to decipher issues (yes - I've
>> tried :-)).
>
> This looks much better now. Maybe just one small suggestion:
> `java.lang.StringCoding#lookupCharset` is used in two places and in both
> places the same exception handling/rethrowing logic is wrapped around the
> invocation. So you could move that logic into the lookupCharset method which
> would simplify call sites. You could even get rid of String.lookupCharset
> method that way.
When you combine the logic of String.lookupCharset:
private static Charset lookupCharset(String charsetName)
throws UnsupportedEncodingException {
Objects.requireNonNull(charsetName);
try {
Charset cs = StringCoding.lookupCharset(charsetName);
if (cs == null) {
throw new UnsupportedEncodingException(charsetName);
}
return cs;
} catch (IllegalCharsetNameException ics) {
throw new UnsupportedEncodingException(charsetName);
}
}
... and StringCoding.lookupCharset:
static Charset lookupCharset(String csn) {
if (Charset.isSupported(csn)) {
try {
return Charset.forName(csn);
} catch (UnsupportedCharsetException x) {
throw new Error(x);
}
}
return null;
}
...you get this:
private static Charset lookupCharset(String charsetName)
throws UnsupportedEncodingException {
Objects.requireNonNull(charsetName);
try {
Charset cs;
inner: {
if (Charset.isSupported(charsetName)) {
try {
cs = Charset.forName(charsetName);
break inner;
} catch (UnsupportedCharsetException x) {
throw new Error(x);
}
}
cs = null;
}
if (cs == null) {
throw new UnsupportedEncodingException(charsetName);
}
return cs;
} catch (IllegalCharsetNameException ics) {
throw new UnsupportedEncodingException(charsetName);
}
}
...and that can be simplified to this:
static Charset lookupCharset(String csn) throws
UnsupportedEncodingException {
Objects.requireNonNull(csn);
try {
return Charset.forName(csn);
} catch (UnsupportedCharsetException | IllegalCharsetNameException x) {
throw new UnsupportedEncodingException(csn);
}
}
which has an additional benefit that it only performs one lookup
(Charset.forName) instead of two (Charset.isSupported & Charset.forName)...
-------------
PR: https://git.openjdk.java.net/jdk/pull/2102
