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-20308 Define a fixed suffix without the ICU version suffix #308

Merged
merged 1 commit into from Dec 13, 2018

Conversation

gvictor
Copy link
Collaborator

@gvictor gvictor commented Dec 11, 2018

  • Add a config macro U_DISABLE_VERSION_SUFFIX to disable version suffix

  • Issue filed: https://unicode-org.atlassian.net/browse/ICU-20308

  • Updated PR title and link in previous line to include Issue number

  • Issue accepted

  • Tests included

  • Documentation is changed or added

@gvictor gvictor changed the title ICU-20308: Define a fixed suffix without the ICU version suffix ICU-20308 Define a fixed suffix without the ICU version suffix Dec 11, 2018
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.

Please add srl295 as a reviewer. GitHub does not seem to allow me to do it.

icu4c/source/common/unicode/uvernum.h Outdated Show resolved Hide resolved
srl295
srl295 previously requested changes Dec 11, 2018
Copy link
Member

@srl295 srl295 left a comment

Choose a reason for hiding this comment

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

what Markus said and requesting docs, otherwise lgtm

icu4c/source/common/unicode/uvernum.h Outdated Show resolved Hide resolved
- Add a config macro U_DISABLE_VERSION_SUFFIX to disable version suffix
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

@gvictor gvictor dismissed srl295’s stale review December 13, 2018 17:56

I have address the comment by markus, and added the doc.

@gvictor
Copy link
Collaborator Author

gvictor commented Dec 13, 2018

Not authorized to merge this PR. Could someone help?

@markusicu
Copy link
Member

I sent Steven a chat message asking if he wants further changes, wants to approve, or is ok with me to dismiss his request for changes. I want to give him some time before I dismiss & merge.

@gvictor
Copy link
Collaborator Author

gvictor commented Dec 13, 2018

Thanks, Markus and Steven!

@markusicu markusicu merged commit 8aa5d23 into unicode-org:master Dec 13, 2018
@markusicu
Copy link
Member

merged. please close the ticket, and probably delete your branch.
thanks for the contribution!

@gvictor gvictor deleted the fixed_suffix branch December 13, 2018 18:58
@sffc
Copy link
Member

sffc commented Apr 8, 2019

This change causes a build breakage on some Chrome builds. Enabling -Wundef exposes the warning. That warning is not enabled in ICU, but it is an error for Chrome.

https://unicode-org.atlassian.net/browse/ICU-20543

BoredOutOfMyGit pushed a commit to codeaurora-unofficial/platform-external-icu that referenced this pull request Dec 29, 2019
…n suffix

The upstream change is pending review.
unicode-org/icu#308

Bug: 117094880
Test: m
Change-Id: If71ba334f13947969335fd6538c0301cf753c7ea
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants