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-22068 Cleanup inconsistent annotations between declarations and d… #2118

Merged
merged 1 commit into from
Sep 8, 2022
Merged

ICU-22068 Cleanup inconsistent annotations between declarations and d… #2118

merged 1 commit into from
Sep 8, 2022

Conversation

cyndyishida
Copy link
Contributor

@cyndyishida cyndyishida commented Jun 22, 2022 •
edited
Loading

…efinitions

This cleans up inconsistent annotations between declared APIs in headers
vs defined implementations in cpp's. This better ensures the API's
referenceable in headers represent what is exposed and defined in the
ultimate binary library's symbol table.

  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22068
  • 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 (current regression tests should suffice)
  • API docs and/or User Guide docs changed or added, if applicable

@cyndyishida cyndyishida force-pushed the tapi/ICUCppCleanupP4 branch from 22cc676 to 09e89ed Compare June 22, 2022 22:39
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/common/unicode/utf_old.h is different
  • icu4c/source/stubdata/BUILD.bazel is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@cyndyishida cyndyishida force-pushed the tapi/ICUCppCleanupP4 branch from 09e89ed to 2a97371 Compare June 23, 2022 05:17
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/common/unicode/utf_old.h is different
  • icu4c/source/common/utf_impl.cpp is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@yumaoka yumaoka assigned richgillam and markusicu and unassigned richgillam Jun 23, 2022
richgillam
richgillam previously approved these changes Jun 24, 2022
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.

FWIW, this all looks good to me (modulo a couple questions). I'm interested in what Markus has to say, though...

@cyndyishida
Copy link
Contributor Author

Waiting on @markusicu feedback before moving forward.

pedberg-icu
pedberg-icu previously approved these changes Jun 30, 2022
View reviewed changes
@pedberg-icu
Copy link
Contributor

@markusicu You probably want to have a look at this one.

@markusicu
Copy link
Member

Is it possible to add "TAPI" to a build bot, so that these kinds of things don't backslide?

@cyndyishida
Copy link
Contributor Author

Is it possible to add "TAPI" to a build bot, so that these kinds of things don't backslide?

I would love to add TAPI to ICU's CI! Unfortunately the tool itself only works out of the box with Xcode. The tool hasn't been completely upstreamed as apart of LLVM (slowly working on this). I'd love to touch base again after we've finished open sourcing TAPI as apart of LLVM's toolchain to get this supported.

@markusicu
Copy link
Member

Also, there is a merge conflict. Please rebase & resolve.

@cyndyishida
ICU-22068 Cleanup inconsistent annotations between declarations and d…
6d76a8a 
…efinitions

This cleans up inconsistent annotations between declared APIs in headers
vs defined implementations in cpp's. This better ensures the API's
referenceable in headers represent what is exposed and defined in the
ultimate binary library's symbol table.
@cyndyishida cyndyishida dismissed stale reviews from pedberg-icu and richgillam via 6d76a8a July 13, 2022 21:26
@cyndyishida cyndyishida force-pushed the tapi/ICUCppCleanupP4 branch from 2a97371 to 6d76a8a Compare July 13, 2022 21:26
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/i18n/collationbuilder.cpp is different
  • icu4c/source/i18n/collationdatabuilder.cpp is different
  • icu4c/source/stubdata/stubdata.h is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

markusicu
markusicu approved these changes Sep 8, 2022
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

@pedberg-icu pedberg-icu merged commit 03b94e9 into unicode-org:main Sep 8, 2022
@srl295 srl295 mentioned this pull request Oct 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jefgen jefgen jefgen left review comments

@richgillam richgillam richgillam left review comments

@markusicu markusicu markusicu approved these changes

@pedberg-icu pedberg-icu pedberg-icu left review comments

Assignees

@markusicu markusicu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cyndyishida @pedberg-icu @markusicu @jefgen @richgillam