Skip to content

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

Merged
merged 1 commit into from
May 26, 2020

Conversation

ammaraskar
Copy link
Member

@ammaraskar ammaraskar commented May 26, 2020

This fixes the following warnings:

Tools\peg_generator\peg_extension\peg_extension.c(127): warning C4477: 'printf' : format string '%4ld' requires an argument of type 'long', but variadic argument 1 has type 'Py_ssize_t'
Tools\peg_generator\peg_extension\peg_extension.c(137): warning C4113: 'PyObject *(__cdecl *)()' differs in parameter lists from 'PyCFunction'
Tools\peg_generator\peg_extension\peg_extension.c(138): warning C4113: 'PyObject *(__cdecl *)()' differs in parameter lists from 'PyCFunction'
Tools\peg_generator\peg_extension\peg_extension.c(139): warning C4113: 'PyObject *(__cdecl *)()' differs in parameter lists from 'PyCFunction'

Parser\tokenizer.c(35): warning C4273: 'PyOS_Readline': inconsistent dll linkage
include\pythonrun.h(184): note: see previous definition of 'PyOS_Readline'

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 C pgen executable and the interpreter:

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'd PyOS_Readline and the PyAPI_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-2019

As 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.

@bedevere-bot
Copy link

🤖 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.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 26, 2020
Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pablogsal pablogsal added the needs backport to 3.9 only security fixes label May 26, 2020
@pablogsal pablogsal merged commit a2bbedc into python:master May 26, 2020
@miss-islington
Copy link
Contributor

Thanks @ammaraskar for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 26, 2020
(cherry picked from commit a2bbedc)

Co-authored-by: Ammar Askar <ammar@ammaraskar.com>
@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label May 26, 2020
@bedevere-bot
Copy link

GH-20408 is a backport of this pull request to the 3.9 branch.

@pablogsal
Copy link
Member

Thank you very much for fixing these and for the detailed analysis 🚀

miss-islington added a commit that referenced this pull request May 26, 2020
(cherry picked from commit a2bbedc)

Co-authored-by: Ammar Askar <ammar@ammaraskar.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants