GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-39355: making Python.h compatible with C++20 compilers by AliyevH · Pull Request #31282 · python/cpython

Conversation

@AliyevH

@AliyevH AliyevH commented Feb 11, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue39355

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Feb 11, 2022
@bedevere-bot bedevere-bot added the awaiting review label Feb 11, 2022
erlend-aasland
erlend-aasland requested changes Feb 11, 2022
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

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.

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 :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Feb 11, 2022
@erlend-aasland erlend-aasland added needs backport to 3.9 labels Feb 11, 2022
@erlend-aasland
erlend-aasland requested a review from vstinner February 11, 2022 18:12
@erlend-aasland

erlend-aasland commented Feb 11, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AliyevH
bpo-39355: making Python.h compatible with C++20 compilers
31708f4 
Renamed module to mod in header files
@AliyevH
AliyevH force-pushed the main branch from bd2bcf5 to 31708f4 Compare February 11, 2022 18:49
@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
@blurb-it
📜🤖 Added by blurb_it.
75e9690
erlend-aasland
erlend-aasland reviewed Feb 11, 2022
View reviewed changes
Comment thread Misc/NEWS.d/next/Core and Builtins/2022-02-11-18-51-11.bpo-39355.rcUPdT.rst Outdated
@@ -0,0 +1 @@
Make Python.h compatible with C++20 compilers

@erlend-aasland erlend-aasland Feb 11, 2022

Copy link
Copy Markdown
Contributor

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.

Nitpick:

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@AliyevH

AliyevH commented Feb 11, 2022

Copy link
Copy Markdown
Contributor Author

@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AliyevH
Update 2022-02-11-18-51-11.bpo-39355.rcUPdT.rst
cb7c264
@erlend-aasland

erlend-aasland commented Feb 11, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
AliyevH added 2 commits February 11, 2022 23:18
@AliyevH
bpo-39355: added a note in What's New - Make Python.h compatible with…
2f4c9f0 
… C++20 compilers
@AliyevH
Merge branch 'main' into main
fd84525
vstinner
vstinner reviewed Feb 28, 2022
View reviewed changes
Comment thread Misc/NEWS.d/next/Core and Builtins/2022-02-11-18-51-11.bpo-39355.rcUPdT.rst
@@ -0,0 +1 @@
Make Python.h compatible with C++20 compilers.

@vstinner vstinner Feb 28, 2022

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@AliyevH AliyevH Mar 4, 2022

Copy link
Copy Markdown
Contributor Author

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.

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 :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Mar 7, 2022

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@AliyevH AliyevH Mar 7, 2022

Copy link
Copy Markdown
Contributor Author

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.

@vstinner I asked question ))

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@AliyevH AliyevH Mar 7, 2022

Copy link
Copy Markdown
Contributor Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Mar 29, 2022

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@AliyevH AliyevH Mar 30, 2022

Copy link
Copy Markdown
Contributor Author

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.

@vstinner thanks for great investigation!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Mar 30, 2022

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Mar 30, 2022

Copy link
Copy Markdown
Member

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.

First step: https://bugs.python.org/issue47164

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
AliyevH and others added 2 commits March 28, 2022 14:01
@AliyevH
Merge branch 'main' into main
c9c587f
@AliyevH
Update 3.11.rst
f7d973b
@vstinner

vstinner commented Mar 29, 2022

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

Reviewers

@vstinner vstinner vstinner left review comments
@erlend-aasland erlend-aasland erlend-aasland requested changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@AliyevH @erlend-aasland @vstinner @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.