Skip to content

Conversation

@AliyevH
Copy link
Contributor

@AliyevHAliyevH commented Feb 11, 2022

Copy link
Contributor

@erlend-aaslanderlend-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.

@AliyevHAliyevH changed the title bpo-39355: Renamed module to pyModule in header filesbpo-39355: making Python.h compatible with C++20 compilersFeb 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
ContributorAuthor

@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
ContributorAuthor

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
ContributorAuthor

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
ContributorAuthor

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
ContributorAuthor

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

Sign up for freeto 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

@AliyevH@erlend-aasland@vstinner@the-knights-who-say-ni@bedevere-bot