-
-
Notifications
You must be signed in to change notification settings - Fork 33.3k
Fix peg_generator compiler warnings under MSVC #20405
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
|
🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 7f546f8 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
pablogsal
left a comment
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.
LGTM
|
Thanks @ammaraskar for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9. |
(cherry picked from commit a2bbedc) Co-authored-by: Ammar Askar <[email protected]>
|
GH-20408 is a backport of this pull request to the 3.9 branch. |
|
Thank you very much for fixing these and for the detailed analysis 🚀 |
(cherry picked from commit a2bbedc) Co-authored-by: Ammar Askar <[email protected]>
This fixes the following warnings:
The changes to
peg_extension.care pretty simple.For the tokenizer.c change, looking through the blame, it looks like
PyOS_Readlinewas extern'd so it could be included in both the old Cpgenexecutable and the interpreter:pgenin bpo-35808, we still used to link withtokenizer.oandmyreadline.o: https://github.com/python/cpython/blob/3.7/Makefile.pre.in#L311)Unfortunately right now this causes a warning due when building the
peg_generatormodule since it uses tokenizer.c as a source file. This causes a mismatch between the extern'dPyOS_Readlineand thePyAPI_FUNC'd version of the functions.Since we're building an extension, this causes pyport.h to set the function as a
__declspec(dllimport)which doesn't match the extern definition without the import declaration. See the page on the warning for more details: https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-1-c4273?view=vs-2019As far as I know, the only supported way of using tokenizer.c is with a Python runtime (since the
#ifndef PGENguards were removed) so this extern should be safe to remove now.