Skip to content
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

bpo-39355: making Python.h compatible with C++20 compilers #31282

Closed
wants to merge 7 commits into from

Conversation

AliyevH
Copy link
Contributor

@AliyevH AliyevH commented Feb 11, 2022

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

Thank you for your PR!

I would strongly suggest to rename to mod instead of pyModule. It is more aligned with the code style of the majority of the CPython code base.

Also:

  1. Please write a NEWS entry (I'm thinking something like "Make Python.h compatible with C++20 compilers", but there may be better ways to present this).
  2. Please add a note in What's New. The text itself can be just a C&P of the NEWS entry.
  3. Please update the PR title to more accurately reflect the change: you are making Python.h compatible with C++20 compilers.

IMO, we should probably backport this to 3.10 and 3.9. I'm adding the labels; if anyone disagrees, please remove them :)

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Feb 11, 2022

BTW, did you test that it is actually possible to use a C++20 compiler for external modules after this change? AFAICS, it should be ok, but assumption has resulted in a lot of strange stuff.

Renamed module to mod in header files
@AliyevH AliyevH changed the title bpo-39355: Renamed module to pyModule in header files bpo-39355: making Python.h compatible with C++20 compilers Feb 11, 2022
@@ -0,0 +1 @@
Make Python.h compatible with C++20 compilers
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick:

Suggested change
Make Python.h compatible with C++20 compilers
Make Python.h compatible with C++20 compilers.

@AliyevH
Copy link
Contributor Author

AliyevH commented Feb 11, 2022

@erlend-aasland Thanks for the corrections !

renamed pyModule to mod.
added misc.
now i don't have C++20 compiler installed and can't test it

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Feb 11, 2022

Thanks for the amendments, Hasan. Could you please also add an entry to What's New. Something like this should suffice (given that this actually is a sufficient change):

diff --git a/Doc/whatsnew/3.11.rst b/Doc/whatsnew/3.11.rst
index 5738745ba1..4cf489ad5e 100644
--- a/Doc/whatsnew/3.11.rst
+++ b/Doc/whatsnew/3.11.rst
@@ -650,6 +650,9 @@ Build Changes
   be removed at some point in the future. (Contributed by Mark Dickinson in
   :issue:`45569`.)
 
+* Python.h is now compatible with C++20 compilers. (Contributed by by Hasan
+  Aliyev in :issue:`39355`.)
+
 
 C API Changes
 =============

I'll see if I can get around to test it using G++ 11.

@@ -0,0 +1 @@
Make Python.h compatible with C++20 compilers.
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind to explain that the change is to avoid the usage of "module" which became a keyword in C++20?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes @vstinner, you are right!

I have mentioned in bugs.python.org, about this change.
After making a little research, i realized that this "module" keyword has meaning with "export"
Sorry i am not c++ expert )) If somebody have knowledge about it, share it plz :)

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand your comment. Do you mean that https://bugs.python.org/issue39355 is not a bug and the Python C API is compatible with C++20?

If there is an issue, please explain the change in NEWS entry. "Make Python.h compatible with C++20 compilers." is not enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vstinner I asked question ))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vstinner it was interesting for me, if "module" keyword has meaning with "export" how gcc will treat "module" without "export" keyword ?

Copy link
Member

Choose a reason for hiding this comment

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

So far, I failed to reproduce https://bugs.python.org/issue39355 issue: using the Python C API in C++ doesn't emit any compiler warning or compiler error. See my test: https://bugs.python.org/issue39355#msg416256

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vstinner thanks for great investigation!

Copy link
Member

Choose a reason for hiding this comment

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

You're welcome. I'm now considering serious to convert #32175 to a real unit test to make sure that the Python C API is compatible with C++.

Copy link
Member

Choose a reason for hiding this comment

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

@vstinner
Copy link
Member

This PR solves a non-problem: https://bugs.python.org/issue39355#msg416257

@vstinner vstinner closed this Mar 29, 2022
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