RFR: JDK-8276302: Locale.filterTags methods ignore actual weight when matching "*" (as if it is 1)
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. ------------- Commit messages: - Locale.filterTags must respect the weight of "*" Changes: https://git.openjdk.java.net/jdk/pull/6789/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6789&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276302 Stats: 121 lines in 2 files changed: 59 ins; 54 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/6789.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6789/head:pull/6789 PR: https://git.openjdk.java.net/jdk/pull/6789
On Fri, 10 Dec 2021 00:02:31 GMT, Daniel Le <duke@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
On Mon, 20 Dec 2021 18:18:17 GMT, Naoto Sato <naoto@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
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.
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 ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/6789/files - new: https://git.openjdk.java.net/jdk/pull/6789/files/85d71141..c3abb980 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6789&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6789&range=00-01 Stats: 17 lines in 2 files changed: 4 ins; 11 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/6789.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6789/head:pull/6789 PR: https://git.openjdk.java.net/jdk/pull/6789
On Wed, 22 Dec 2021 07:33:55 GMT, Daniel Le <duke@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.
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
I've addressed most of your comments in https://github.com/openjdk/jdk/pull/6789#pullrequestreview-836689415 except for https://github.com/openjdk/jdk/pull/6789/files#r772577336. Thank you. ------------- PR: https://git.openjdk.java.net/jdk/pull/6789
On Wed, 22 Dec 2021 07:33:55 GMT, Daniel Le <duke@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.
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
Thanks for the changes. Those all look good. I will sponsor this PR once it is integrated. ------------- Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6789
On Fri, 10 Dec 2021 00:02:31 GMT, Daniel Le <duke@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.
This pull request has now been integrated. Changeset: 87cc4e50 Author: Daniel Le <greenrecyclebin@gmail.com> Committer: Naoto Sato <naoto@openjdk.org> URL: https://git.openjdk.java.net/jdk/commit/87cc4e5009f6b900c62a91dda1c2f98e4821... Stats: 138 lines in 2 files changed: 63 ins; 65 del; 10 mod 8276302: Locale.filterTags methods ignore actual weight when matching "*" (as if it is 1) Reviewed-by: naoto ------------- PR: https://git.openjdk.java.net/jdk/pull/6789
participants (2)
-
Daniel Le
-
Naoto Sato