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-22954 USet C++ iterator return std::u16string; header-only LocalPointer #3295

Merged
merged 5 commits into from
Dec 20, 2024
Merged

ICU-22954 USet C++ iterator return std::u16string; header-only LocalPointer #3295

merged 5 commits into from
Dec 20, 2024

Conversation

markusicu
Copy link
Member

@markusicu markusicu commented Dec 10, 2024 •
edited
Loading

  • ICU-22954 Build error in uset.h when U_SHOW_CPLUSPLUS_HEADER_API is true but U_SHOW_CPLUSPLUS_API is false
    • USet C++ iterator return std::u16string instead of a not-header-only icu::UnicodeString
    • Change USetElements to draft ICU 77 because of this change
    • Make the intltest C++ test framework work without U_SHOW_CPLUSPLUS_API by
      • switching from UnicodeString arguments to std::u16string_view
      • decoupling IcuTestErrorCode from the DLL-exported class ErrorCode
    • Test header-only USet C++ iterators in an intltest file with U_SHOW_CPLUSPLUS_API=0
  • ICU-22980 make localpointer.h header-only compatible, and parts of char16ptr.h
    • Add U_ICU_NAMESPACE_OR_INTERNAL = icu or icu::internal
    • Change localpointer.h to be header-only and work with just U_SHOW_CPLUSPLUS_HEADER_API
    • (Const)Char16Ptr need to remain DLL-exported because they are baked into DLL-exported APIs in UnicodeString member function signatures

Best to review one commit at a time.

Checklist

  • Required: Issue filed: ICU-22954
  • 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

ALLOW_MANY_COMMITS=true

@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

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

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

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@markusicu markusicu changed the title ICU-22954 USet C++ iterator return std::u16string ICU-22954 USet C++ iterator return std::u16string; header-only LocalPointer Dec 13, 2024
roubert
roubert requested changes Dec 17, 2024
View reviewed changes
icu4c/source/test/intltest/headeronlytest.cpp Outdated Show resolved Hide resolved
icu4c/source/test/intltest/headeronlytest.cpp Outdated Show resolved Hide resolved
icu4c/source/test/intltest/headeronlytest.cpp Outdated Show resolved Hide resolved
icu4c/source/test/intltest/headeronlytest.cpp Outdated Show resolved Hide resolved
icu4c/source/test/intltest/Makefile.in Outdated Show resolved Hide resolved
icu4c/source/test/intltest/usettest.cpp Outdated Show resolved Hide resolved
icu4c/source/test/intltest/headeronlytest.cpp Outdated Show resolved Hide resolved
icu4c/source/common/unicode/uset.h Show resolved Hide resolved
icu4c/source/common/unicode/uset.h Outdated Show resolved Hide resolved
@markusicu
Copy link
Member Author

@richgillam sort-of-approved the API change proposal. I don't expect any controversy.

It would be lovely to get this PR reviewed (I suggest one commit at a time) and approved so that I can merge it soon after the upcoming ICU-TC meeting.

@markusicu
Copy link
Member Author

Oops, this window hadn't refreshed before I sent my previous comment -- now it has. Will respond to feedback.

@markusicu
Copy link
Member Author

Will respond to feedback.

done

roubert
roubert requested changes Dec 18, 2024
View reviewed changes
icu4c/source/test/intltest/headeronlytest.cpp Outdated Show resolved Hide resolved
icu4c/source/test/intltest/Makefile.in Outdated Show resolved Hide resolved
icu4c/source/test/intltest/headeronlytest.cpp Outdated Show resolved Hide resolved
icu4c/source/test/intltest/headeronlytest.cpp Outdated Show resolved Hide resolved
icu4c/source/common/unicode/uset.h Show resolved Hide resolved
@markusicu markusicu requested a review from roubert December 18, 2024 20:43
@markusicu
Copy link
Member Author

feedback addressed, PTAL

roubert
roubert previously requested changes Dec 19, 2024
View reviewed changes
icu4c/source/test/intltest/headeronlytest.cpp Outdated Show resolved Hide resolved
icu4c/source/test/intltest/headeronlytest.cpp Outdated Show resolved Hide resolved
icu4c/source/test/intltest/headeronlytest.cpp Outdated Show resolved Hide resolved
@markusicu
Copy link
Member Author

FYI: The ICU-TC approved the API change proposal.

@markusicu
Copy link
Member Author

Thanks Robin!

@roubert I have addressed your feedback. I don't want to fiddle further with the sample code in this PR. If I don't hear back from you during Japan business hours, then I plan to dismiss your change request and merge.

Except, I will first try to squash the feedback commits into the regular ones, so that this is down to a small number of somewhat meaningful commits.

@richgillam please speak up if you disagree.

@richgillam
Copy link
Contributor

@richgillam please speak up if you disagree.

No, I think I'm happy...

@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

@markusicu markusicu dismissed roubert’s stale review December 20, 2024 01:01

feedback addressed

@markusicu markusicu merged commit e3bc073 into unicode-org:main Dec 20, 2024
94 checks passed
@markusicu markusicu deleted the uset-iter-no-unistr branch December 20, 2024 01:40
markusicu added a commit to markusicu/icu that referenced this pull request Dec 23, 2024
@markusicu
ICU-22954 Revert PR unicode-org#3295 make all LocalXyzPointer header-…
f7a4298 
…only

This reverts commit 5bdb4c4.

Making LocalPointer header-only, with a different namespace when compiling internally,
turned out to be problematic.
markusicu added a commit to markusicu/icu that referenced this pull request Dec 23, 2024
@markusicu
ICU-22954 Partially revert PR unicode-org#3295 U_ICU_NAMESPACE_OR_INT…
5b63469 
…ERNAL, header-only localpointer

This partially reverts commit 3527b3d.

Making LocalPointer header-only, with a different namespace when compiling internally,
turned out to be problematic.
@markusicu markusicu mentioned this pull request Dec 23, 2024
Merged
7 tasks
markusicu added a commit that referenced this pull request Dec 23, 2024
@markusicu
ICU-22954 Revert PR #3295 make all LocalXyzPointer header-only
38b6d7f 
This reverts commit 5bdb4c4.

Making LocalPointer header-only, with a different namespace when compiling internally,
turned out to be problematic.
markusicu added a commit that referenced this pull request Dec 23, 2024
@markusicu
ICU-22954 Partially revert PR #3295 U_ICU_NAMESPACE_OR_INTERNAL, head…
c0a3fe1 
…er-only localpointer

This partially reverts commit 3527b3d.

Making LocalPointer header-only, with a different namespace when compiling internally,
turned out to be problematic.
@markusicu markusicu mentioned this pull request Dec 23, 2024
Open
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roubert roubert roubert left review comments

@eggrobin eggrobin eggrobin approved these changes

@richgillam richgillam Awaiting requested review from richgillam

Assignees

@eggrobin eggrobin

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@markusicu @richgillam @roubert @eggrobin