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

<stdatomic.h>: Fix preprocessor logic #2615

Merged

Conversation

barcharcraz
Copy link
Member

Because this header needs to work in C and C++, we needed to move the check for a non-broken preprocessor outside of yvals_core, but we didn't actually skip the whole header if it failed!

This PR fixes that. In addition, it:

  • Reverses the conditional logic to avoid needing to use an "else" branch
  • Changes the #if _STL_COMPILER_PREPROCESSOR to _STL_INTERNAL_STATIC_ASSERT(_STL_COMPILER_PREPROCESSOR);

@barcharcraz barcharcraz requested a review from a team as a code owner March 23, 2022 20:17
@CaseyCarter CaseyCarter added the bug Something isn't working label Mar 23, 2022
stl/inc/stdatomic.h Outdated Show resolved Hide resolved
stl/inc/stdatomic.h Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added enhancement Something can be improved and removed bug Something isn't working labels Mar 23, 2022
@StephanTLavavej
Copy link
Member

Relabeling - I don't think that this was a bug, strictly speaking, as for non-compiler tools we'd skip the C check, include yvals.h (which would skip its contents), and then skip the rest of the file - which should work (if my vague understanding of the non-compiler tools is correct).

I do believe that simply skipping the whole thing is an improvement.

@barcharcraz
Copy link
Member Author

Relabeling - I don't think that this was a bug, strictly speaking, as for non-compiler tools we'd skip the C check, include yvals.h (which would skip its contents), and then skip the rest of the file - which should work (if my vague understanding of the non-compiler tools is correct).

I do believe that simply skipping the whole thing is an improvement.

Good point, the comment was confusing though and it caused me to write my own bug when modifying the header, so I would also say it's an improvement

@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 1b6fa43 into microsoft:main Mar 28, 2022
@StephanTLavavej
Copy link
Member

Thanks for simplifying this code! ✨ 😸 ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants