On Mon, 20 Dec 2021 18:18:17 GMT, Naoto Sato <[email protected]> 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
