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-22435 Add C API for Locale #2531

Merged
merged 1 commit into from
Aug 17, 2023
Merged

ICU-22435 Add C API for Locale #2531

merged 1 commit into from
Aug 17, 2023

Conversation

FrankYFTang
Copy link
Contributor

@FrankYFTang FrankYFTang commented Jul 21, 2023 •
edited
Loading

Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22435
  • Required: The PR title must be prefixed with a JIRA Issue number.
  • Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • Required: Each commit message must be prefixed with a JIRA Issue number.
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

@FrankYFTang FrankYFTang added incomplete Needs work; do not approve/merge as is. api Adds or changes public API. labels Jul 21, 2023
@FrankYFTang FrankYFTang force-pushed the ICU-22435-C-localeAPI branch from 9fefa1a to 43a4ed7 Compare July 21, 2023 17:05
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/common/unicode/uloc.h is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@FrankYFTang FrankYFTang force-pushed the ICU-22435-C-localeAPI branch from fe6d98c to 4391c79 Compare August 1, 2023 21:29
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/common/unicode/ulocale.h is different
  • icu4c/source/test/cintltst/cintltst.vcxproj is now changed in the branch
  • icu4c/source/test/cintltst/cintltst.vcxproj.filters is now changed in the branch
  • icu4c/source/test/cintltst/cutiltst.c is now changed in the branch
  • icu4c/source/test/cintltst/Makefile.in is now changed in the branch
  • icu4c/source/test/cintltst/ulocaletst.c is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@FrankYFTang
Copy link
Contributor Author

ICU TC approved the API proposal on 2023-08-03 as implemented in this PR. However, please do NOT review this PR yet since I am still figuring out how to port C++/Java test to test these C interface.

@FrankYFTang FrankYFTang force-pushed the ICU-22435-C-localeAPI branch from 4391c79 to ef6116c Compare August 8, 2023 21:48
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/common/BUILD.bazel is different
  • icu4c/source/common/common_uwp.vcxproj is different
  • icu4c/source/common/common.vcxproj is different
  • icu4c/source/common/common.vcxproj.filters is different
  • icu4c/source/common/sources.txt is different
  • icu4c/source/test/cintltst/cintltst.vcxproj is different
  • icu4c/source/test/cintltst/cintltst.vcxproj.filters is different
  • icu4c/source/test/cintltst/cutiltst.c is different
  • icu4c/source/test/cintltst/Makefile.in is different
  • icu4c/source/test/depstest/dependencies.txt is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@FrankYFTang FrankYFTang changed the title ICU-22435 Add C API for Locale ICU-22435 Add C API for Locale (incomplete, tests not added yet) Aug 8, 2023
@FrankYFTang FrankYFTang force-pushed the ICU-22435-C-localeAPI branch from ef6116c to 3ca1d16 Compare August 16, 2023 03:11
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/common/ulocale.cpp is different
  • icu4c/source/common/unicode/ulocale.h is different
  • icu4c/source/test/cintltst/ulocaletst.c is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@FrankYFTang FrankYFTang changed the title ICU-22435 Add C API for Locale (incomplete, tests not added yet) ICU-22435 Add C API for Locale Aug 16, 2023
@FrankYFTang FrankYFTang added waiting-on-reviewer and removed incomplete Needs work; do not approve/merge as is. labels Aug 16, 2023
@FrankYFTang
Copy link
Contributor Author

All necessary tests are added. Please start review. Thanks

richgillam
richgillam previously approved these changes Aug 16, 2023
View reviewed changes
Copy link
Contributor

@richgillam richgillam left a comment

Choose a reason for hiding this comment

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

Looks good. I have a few nits and one significant API-design issue it might be too late to do anything about.

icu4c/source/common/ulocale.cpp Outdated Show resolved Hide resolved
icu4c/source/common/ulocale.cpp Outdated Show resolved Hide resolved
icu4c/source/common/ulocale.cpp Outdated Show resolved Hide resolved
icu4c/source/common/ulocale.cpp Show resolved Hide resolved
icu4c/source/common/ulocale.cpp Show resolved Hide resolved
icu4c/source/common/unicode/ulocale.h Outdated Show resolved Hide resolved
icu4c/source/common/unicode/ulocale.h Outdated Show resolved Hide resolved
icu4c/source/common/unicode/ulocale.h Show resolved Hide resolved
icu4c/source/test/cintltst/ulocaletst.c Show resolved Hide resolved
icu4c/source/test/cintltst/ulocaletst.c Outdated Show resolved Hide resolved
@FrankYFTang FrankYFTang requested a review from richgillam August 17, 2023 00:42
@FrankYFTang
Copy link
Contributor Author

@richgillam I think I addressed all your comments PTAL

richgillam
richgillam reviewed Aug 17, 2023
View reviewed changes
Copy link
Contributor

@richgillam richgillam left a comment

Choose a reason for hiding this comment

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

assertSuccess() returns true if the error code passed U_SUCCESS and false if it passed U_FAILURE, so when you're using it, you don't need to do those checks yourself, and you want to be sure to check the return value in cases where the test depends on the previous call having succeeded.

I think there are a couple of other small mistakes in here.

But this looks a lot better! Thanks a lot for the changes!

icu4c/source/common/ulocale.cpp Outdated Show resolved Hide resolved
icu4c/source/common/ulocale.cpp Outdated Show resolved Hide resolved
icu4c/source/test/cintltst/ulocaletst.c Show resolved Hide resolved
icu4c/source/test/cintltst/ulocaletst.c Outdated Show resolved Hide resolved
icu4c/source/test/cintltst/ulocaletst.c Outdated Show resolved Hide resolved
icu4c/source/test/cintltst/ulocaletst.c Outdated Show resolved Hide resolved
icu4c/source/test/cintltst/ulocaletst.c Outdated Show resolved Hide resolved
@FrankYFTang FrankYFTang requested a review from richgillam August 17, 2023 01:55
@FrankYFTang
Copy link
Contributor Author

@richgillam PTAL, I correct the assertSuccess based on your comments

richgillam
richgillam approved these changes Aug 17, 2023
View reviewed changes
Copy link
Contributor

@richgillam richgillam left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you for the changes!

@FrankYFTang FrankYFTang force-pushed the ICU-22435-C-localeAPI branch from d0be214 to 7470bd3 Compare August 17, 2023 18:18
@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 FrankYFTang merged commit 27181e3 into unicode-org:main Aug 17, 2023
@FrankYFTang FrankYFTang deleted the ICU-22435-C-localeAPI branch August 17, 2023 19:15
catamorphism pushed a commit to catamorphism/icu that referenced this pull request Nov 1, 2023
@FrankYFTang @catamorphism
ICU-22435 Add C API for Locale
3098f1e 
See unicode-org#2531
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richgillam richgillam richgillam approved these changes

@garywade garywade Awaiting requested review from garywade

Assignees
No one assigned
Labels
api Adds or changes public API. waiting-on-reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@FrankYFTang @richgillam