On Fri, 1 May 2026 20:20:20 GMT, Naoto Sato <[email protected]> wrote:
>> src/java.base/share/classes/sun/util/locale/provider/LocaleResources.java
>> line 660:
>>
>>> 658: var inputSkeletons = new HashMap<String, Map<String,
>>> String>>();
>>> 659: Pattern p = Pattern.compile("([^:]+):([^;]+);");
>>> 660: if
>>> (LocaleProviderAdapter.forType(LocaleProviderAdapter.Type.CLDR) instanceof
>>> ResourceBundleBasedAdapter rbba) {
>>
>> Since the new code relies on matching the results of
>> `LocaleProviderAdapter.forType(LocaleProviderAdapter.Type.CLDR)`, I think
>> there should be a reasonable `InternalError` or the sorts in an `else` block
>> (or perhaps write it as an inverted `if`). Writing it this way makes it
>> clear to the reader that returning an empty map is not a possible
>> intentional path and also ensures an empty map isn't silently returned by
>> the method in the case of an error (even if unlikely).
>
> Since input skeletons are not mandatory elements, theoretically it could be
> possible that CLDR data do not include it (very unlikely though). So instead
> of throwing an error here, defaulting to "h" at the calling site, even if the
> map is empty, would be desirable.
Hmm, I was not talking about whether the CLDR data includes the input
skeletons, but the adapter itself.
As I understand it, the old code only ever constructed`LocaleResources` from a
`ResourceBundleBasedAdapter` adapter. The new code introduces a fallback for
the case when the CLDR adapter is not a `ResourceBundleBasedAdapter`. I am fine
with it if it is intentional, I just noticed it differed from the original
semantics.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/31006#discussion_r3175203840
