-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
PEP 7: Don't allow MSVC extensions #758
Comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@erikjanss In my interpretation, this is already implied as the PEP mentions ANSI/ISO standard C (though since the list of C99 features was added, it became a little less clear). |
@encukou Up to now I only found one (I have not yet been able to go through all files though), Modules/socketmodule.c line 562 :
uses __pragma, which appears to be a MSVC extension : |
Can you file a CPython pull request to properly conditionalize that? I think it will be useful to first have a PR discussion on this particular case, and then to clarify the PEP based on the PR discussion. |
I made a PR for this : The associated issue is 34581 |
@encukou can this issue be closed? The linked PR python/cpython#9067 was merged in 2018, but there didn't seem to be any discussion there, or on bpo-34581. A |
Perhaps it would be best to revise the sentence to state "Don't use any non-standard, compiler-specific extensions," since this would presumably apply to LLVM too, and any other compilers. It may be implied by "ISO-standard C", but it only seems reasonable to either not call out one particular compiler, or explicitly clarify that it applies to all compilers—and "explicit is better than implicit", after all. |
IMO, it's good to point out GCC explicitly -- GCC users are/were the ones using "their" extensions, and getting away with it since most CI-style checks use(d) GCC. (It's, arguably, most straightforward to set up.) |
The reason I made this request was that we tried make CPython compile for windows with mingw (as many others did before). As it is right now, it requires a good amount of patches, to even get the code base to compile. But most of those patches are simply cleaning up the use of 'ifdef MS_WINDOWS', since in lots of places it is assumed that compiling to target windows is the same thing as compiling with MSVC. In the end, we got it working 'good enough' for our case, but decided that from a maintenance perspective, it was better to rewrite our code in C++ and have it compile with ease, than to keep looking for those 'ifdef' issues appearing in the code base, and making PR's to change them. Making it explicit that using those MSVC should be guarded explicitly might make such efforts in the future easier. |
Yeah, historically this makes sense, especially before LLVM came onto the scene.
Sure, happy to. Will do. |
Thanks; opened as #2263 , |
…hon#758 Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
#2263) Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
PEP 7 has this sentence :
I would propose to change this to :
Reason : using MSVC extensions in the C-code makes is harder to reuse the code or do experimental work on the code using different toolchains or make the code portable to other toolchains, which could be a long term disadvantage.
An exemption could be made for the
PC/pyconfig.h
file as this file is more or less MSVC specific.The text was updated successfully, but these errors were encountered: