<i18n dev> RFR: JDK-8276302: Locale.filterTags methods ignore actual weight when matching "*" (as if it is 1) [v2]

[Daniel Le]

On Mon, 20 Dec 2021 18:18:17 GMT, Naoto Sato <naoto at openjdk.org> wrote:

>> Daniel Le has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Address most review comments
>>   
>>   - Include "// change to lowercase for case-insensitive matching" and "//
>>     preserving the case of the input tag" in the if branch in the for loop
>>     of LocaleMatcher.{filterBasic,filterExtended}
>>   
>>   - Remove LocaleMatcher.removeDuplicates since is it no longer used
>>   
>>   - For Bug7069824.java, add 8276302 to the @bug tag and update the
>>     copyright year to 2021
>>   
>>   https://github.com/openjdk/jdk/pull/6789#pullrequestreview-836689415
>
> 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?

Two reasons:
  - It's consistent with the `else`  branch in the `for` loop of `LocaleMatcher.{filterBasic,filterExtended}` (main)
  - I thought that `caseInsensitiveMatch` being first made it clear the need for `lowerCaseTag` but now noticed that the comment was meant for `{shouldIgnoreFilterBasicMatch,shouldIgnoreFilterExtendedMatch}` instead

> 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).

I've tried to follow your suggestion but I think this comment is no longer necessary. (The tags are no longer removed and no collections are updated.)

I had noticed that this comment could not be added back verbatim to the modified code. Hence, I tried to rewrite it. However, I noticed that the comments for `LocaleMatcher{shouldIgnoreFilterBasicMatch,shouldIgnoreFilterExtendedMatch}` clearly covers what we wanted to highlight here.

Let me know if you disagree together with a comment that you'd like me to include verbatim.

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

Done.

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

Done.

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

Done.

-------------

PR: https://git.openjdk.java.net/jdk/pull/6789