-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
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. |
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 <ammar@ammaraskar.com>
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 <ammar@ammaraskar.com>
This fixes the following warnings:
The changes to
peg_extension.c
are pretty simple.For the tokenizer.c change, looking through the blame, it looks like
PyOS_Readline
was extern'd so it could be included in both the old Cpgen
executable and the interpreter:pgen
in bpo-35808, we still used to link withtokenizer.o
andmyreadline.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_generator
module since it uses tokenizer.c as a source file. This causes a mismatch between the extern'dPyOS_Readline
and 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 PGEN
guards were removed) so this extern should be safe to remove now.