On Fri, 10 Dec 2021 00:02:31 GMT, Daniel Le <d...@openjdk.java.net> wrote:
> Locale.filterTags methods ignore actual weight when matching "*" (as if
> it is 1) because LocaleMatcher.{filterBasic,filterExtended} do so.
>
> Fix the bug and add regression test cases for it as well as existing
> behavior.
Thanks for the fix. Looks good overall. Some minor comments follow. Also for
the test case,
- add the bug id `8276302` to the `@bug` tag.
- modify the copyright year to 2021.
src/java.base/share/classes/sun/util/locale/LocaleMatcher.java line 131:
> 129:
> 130: if (!caseInsensitiveMatch(list, lowerCaseTag)
> 131: && !shouldIgnoreFilterBasicMatch(zeroRanges,
> lowerCaseTag)) {
Is there any reason to flip the evaluation order of the `if` statement?
src/java.base/share/classes/sun/util/locale/LocaleMatcher.java line 157:
> 155: * returning all the tags, remove those which are matching the range
> with
> 156: * quality weight q=0.
> 157: */
Please keep the comments in the modified code (except the `*` part). Although
it's OK to remove this method as it eliminates extra `ArrayList` allocation,
please keep the comments in the modified code (except the `*` part).
src/java.base/share/classes/sun/util/locale/LocaleMatcher.java line 161:
> 159: List<LanguageRange> zeroRange, Collection<String> tags) {
> 160: if (zeroRange.isEmpty()) {
> 161: tags = removeDuplicates(tags);
Can `removeDuplicates()` now be removed? There seems to be no usage.
src/java.base/share/classes/sun/util/locale/LocaleMatcher.java line 167:
> 165: List<String> matchingTags = new ArrayList<>();
> 166: for (String tag : tags) {
> 167: // change to lowercase for case-insensitive matching
Applies to this comment.
src/java.base/share/classes/sun/util/locale/LocaleMatcher.java line 171:
> 169: if (!shouldIgnoreFilterBasicMatch(zeroRange, lowerCaseTag)
> 170: && !caseInsensitiveMatch(matchingTags,
> lowerCaseTag)) {
> 171: matchingTags.add(tag); // preserving the case of the
> input tag
And this comment, too.
-------------
PR: https://git.openjdk.java.net/jdk/pull/6789
