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

Avoid conflict with ICU macroizing _No #1570

Merged
merged 1 commit into from
Jan 20, 2021

Conversation

StephanTLavavej
Copy link
Member

#1543 added:

enum class _Case_sensitive : bool { _No, _Yes };

This conflicts with an ICU header that macroizes _No, reported as ICU-21461. This breaks Tensorflow, MySQL, and PDFium in our Real World Code test harness.

While ICU should avoid macroizing these reserved identifiers, the STL should avoid breaking RWC, so this PR renames _No to _Nope. We had previously used this identifier in <memory>, but apparently nobody noticed/complained because no projects were including the conflicting headers in the conflicting order. (There are other conflicts like this: _Pi in <complex>, _Cc in <limits>, and _Mn in <regex>.)

For consistency, I am changing all of the _No occurrences. I didn't think it was necessary to make _Yes sound more parallel (_Nope/_Yep or _Nay/_Yea or whatever). At this time, I don't think it's necessary for the STL to rename the other conflicting identifiers, since the STL is allowed to use them, and they aren't causing RWC breaks.

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Jan 16, 2021
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner January 16, 2021 00:24
@StephanTLavavej StephanTLavavej self-assigned this Jan 19, 2021
@StephanTLavavej StephanTLavavej merged commit 014f1d0 into microsoft:master Jan 20, 2021
@StephanTLavavej StephanTLavavej deleted the no branch January 20, 2021 02:58
@StephanTLavavej
Copy link
Member Author

Upstream fix: unicode-org/icu#1555

It will take some time for this fix to flow into Tensorflow, MySQL, and PDFium, but this is great news. 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants