-
-
Couldn't load subscription status.
- Fork 33.2k
gh-135927: Fix MSVC Clatest C builds #135935
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
Conversation
|
Verified locally that building with clang-cl also works. |
|
Change looks fine to me, though I haven't dug into why the availability differs. If this works, then I guess it's fine. |
|
Thanks Steve! |
|
I am on Mobile, but LGTM |
| // to prevent C++ compiler warnings. On C23 and newer and on C++11 and newer, | ||
| // _Py_NULL is defined as nullptr. | ||
| #if (defined (__STDC_VERSION__) && __STDC_VERSION__ > 201710L) \ | ||
| #if (defined(__GNUC__) || defined(__clang__)) && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there are other C compilers in the wild, it would be better to check for !defined(_MSC_VER) instead of being specific about GCC and clang.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm you're right. Do you want to open a PR for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created #135987 : can you test it with MSC with /std:clatest?
The problem is that we are checking for the latest C assuming only GCC and Clang supports it, but in reality MSVC supports it (on some configurations) too.
The fix is to account for MSVC.