Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ICU-20478 Sort variant in (for|to)LanguageTag of icu::Locale and ULocale #836

Merged
merged 1 commit into from Oct 28, 2019
Merged

ICU-20478 Sort variant in (for|to)LanguageTag of icu::Locale and ULocale #836

merged 1 commit into from Oct 28, 2019

Conversation

FrankYFTang
Copy link
Contributor

Checklist

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/test/cintltst/cloctst.c is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/common/uloc_tag.cpp is different
  • icu4c/source/test/cintltst/cloctst.c is different
  • icu4c/source/test/intltest/loctest.cpp is different
  • icu4j/main/classes/core/src/com/ibm/icu/impl/locale/InternalLocaleBuilder.java is now changed in the branch
  • icu4j/main/tests/core/src/com/ibm/icu/dev/test/util/ULocaleTest.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@FrankYFTang FrankYFTang changed the title ICU-20478 sort variant in toLanguageTag of icu::Locale and ULocale ICU-20478 sort variant in (for|to)LanguageTag of icu::Locale and ULocale Sep 20, 2019
@FrankYFTang FrankYFTang changed the title ICU-20478 sort variant in (for|to)LanguageTag of icu::Locale and ULocale ICU-20478 Sort variant in (for|to)LanguageTag of icu::Locale and ULocale Sep 21, 2019
@markusicu markusicu self-assigned this Sep 25, 2019
markusicu
markusicu requested changes Oct 21, 2019
View reviewed changes
icu4c/source/common/uloc_tag.cpp Outdated
for (VariantListEntry* var1 = first; var1 != NULL; var1 = var1->next) {
for (VariantListEntry* var2 = var1->next; var2 != NULL; var2 = var2->next) {
// Swap var1->variant and var2->variant.
if (strcmp(var1->variant, var2->variant) > 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the ICU4C library code, we want to use uprv_str...() functions not raw str...().

In this case, we would also get inconsistent sort orders for EBCDIC vs. ASCII, which would make your test cases fail on an EBCDIC machine, so actually the right one to use is uprv_compareInvCharsAsAscii(s1, s2) from internal uinvchar.h. (EBCDIC sorts digits after letters.)

(This is not a concern in Java which uses Unicode strings.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL adddressed!

markusicu
markusicu reviewed Oct 28, 2019
View reviewed changes
Copy link
Member

@markusicu markusicu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm please squash

@markusicu
Copy link
Member

FYI remember that if you following the "Details" link on the jira-ticket checker you can squash from the UI there

@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@FrankYFTang
Copy link
Contributor Author

please approve after the force-push. Thanks

markusicu
markusicu approved these changes Oct 28, 2019
View reviewed changes
Copy link
Member

@markusicu markusicu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm tnx

@FrankYFTang FrankYFTang merged commit 84f6735 into unicode-org:master Oct 28, 2019
@FrankYFTang FrankYFTang deleted the ICU-20478 branch October 28, 2019 21:57
jaredtao pushed a commit to QtSkia/icu that referenced this pull request Dec 16, 2019
@FrankYFTang @jungshik
Sort variants in locale code.
88ea42a 
  - upstream bug:
    https://unicode-org.atlassian.net/browse/ICU-20478
  - upstream PR:
    unicode-org/icu#836

Bug: v8:9741
Change-Id: I92bed94b7c729d730ca849189a99ffe7906a310e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/deps/icu/+/1889172
Reviewed-by: Jungshik Shin <jshin@chromium.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markusicu markusicu markusicu approved these changes

@sven-oly sven-oly Awaiting requested review from sven-oly

@yumaoka yumaoka Awaiting requested review from yumaoka

Assignees

@markusicu markusicu

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@FrankYFTang @markusicu @sffc