Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.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 ourterms of service andprivacy 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. |
a88b49c intopython:mainUh oh!
There was an error while loading.Please reload this page.
Thanks Steve! |
I am on Mobile, but LGTM |
| @@ -49,7 +49,8 @@ | |||
| // Static inline functions should use _Py_NULL rather than using directly NULL | |||
| // 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?
Uh oh!
There was an error while loading.Please reload this page.
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.