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.

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

ICU-22936 Replace all ICU4C code that uses UBool as an integer #3232

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

roubert
Copy link
Member

@roubert roubert commented Oct 3, 2024

Checklist

  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22936
  • Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
  • 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. Example: "ICU-1234 Fix xyz"
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

@roubert roubert marked this pull request as ready for review October 3, 2024 12:42
@markusicu markusicu self-assigned this Oct 3, 2024
eggrobin
eggrobin previously approved these changes Oct 3, 2024
icu4c/source/tools/toolutil/ucm.cpp Outdated Show resolved Hide resolved
richgillam
richgillam previously approved these changes Oct 3, 2024
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.

LGTM. A couple nits you can ignore.

// Use two variables so that getMergedCollationKey() is always called for both strings.
if (UBool prev = getMergedCollationKey(prevString.getBuffer(), prevString.length(), prevKey, errorCode),
curr = getMergedCollationKey(s.getBuffer(), s.length(), key, errorCode);
prev || curr || errorCode.isFailure()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it'd read better if we just moved the definitions of prev and curr out of the condition of the if statement.

Copy link
Member

Choose a reason for hiding this comment

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

optional: I agree that if we can't use the bit-wise OR any more then pulling out the two variables, like in the other file, is more readable. Mostly because of the fairly long function calls.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe, but I don't think that's something that'd be worth spending another round of review/approval on.

icu4c/source/test/cintltst/creststn.c Outdated Show resolved Hide resolved
icu4c/source/i18n/scriptset.cpp Show resolved Hide resolved
icu4c/source/i18n/scriptset.cpp Show resolved Hide resolved
icu4c/source/test/cintltst/creststn.c Outdated Show resolved Hide resolved
icu4c/source/test/cintltst/cucdtst.c Outdated Show resolved Hide resolved
icu4c/source/test/cintltst/cloctst.c Show resolved Hide resolved
icu4c/source/test/cintltst/udatatst.c Show resolved Hide resolved
icu4c/source/test/intltest/caltest.cpp Show resolved Hide resolved
icu4c/source/test/intltest/collationtest.cpp Show resolved Hide resolved
icu4c/source/tools/toolutil/ucm.cpp Outdated Show resolved Hide resolved
icu4c/source/tools/toolutil/ucm.cpp Outdated Show resolved Hide resolved
@roubert roubert dismissed stale reviews from richgillam and eggrobin via d2c7558 October 4, 2024 13:37
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.

thanks!

// Use two variables so that getMergedCollationKey() is always called for both strings.
if (UBool prev = getMergedCollationKey(prevString.getBuffer(), prevString.length(), prevKey, errorCode),
curr = getMergedCollationKey(s.getBuffer(), s.length(), key, errorCode);
prev || curr || errorCode.isFailure()) {
Copy link
Member

Choose a reason for hiding this comment

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

optional: I agree that if we can't use the bit-wise OR any more then pulling out the two variables, like in the other file, is more readable. Mostly because of the fairly long function calls.

@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

@roubert roubert merged commit 6ca2885 into unicode-org:main Oct 7, 2024
94 checks passed
@roubert roubert deleted the 22936 branch October 7, 2024 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants