GeezerVison

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

GeezerVison is showing github.com — gh-107298: Document doesn't link to PyAPI_DATA() by vstinner · Pull Request #109236 · python/cpython

Conversation

vstinner commented Sep 10, 2023 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

Remove links to PyAPI_FUNC() and PyAPI_DATA() macros since they are
not documented. These macros should only be used to define the Python
C API. They should not be used outside Python code base.

Document PyMODINIT_FUNC macro.

Issue: Fix Sphinx warnings in the C API documentation #107298

📚 Documentation preview 📚: https://cpython-previews--109236.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner requested review from corona10 and erlend-aasland as code owners

September 10, 2023 20:12

bedevere-bot added awaiting core review docs

Documentation in the Doc dir

skip news labels

Sep 10, 2023

bedevere-bot mentioned this pull request

Sep 10, 2023

Fix Sphinx warnings in the C API documentation #107298

Closed

vstinner mentioned this pull request

Sep 10, 2023

gh-107298: Document PyAPI_DATA() macro #109129

Closed

vstinner commented Sep 10, 2023

Copy link

Copy Markdown

Member Author

cc @hugovk @serhiy-storchaka

All reactions

Sorry, something went wrong.

Uh oh!

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

hugovk approved these changes

Sep 11, 2023

View reviewed changes

vstinner commented

Sep 11, 2023

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

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

Oh! There is still a warning in Doc/whatsnew/2.3.rst: c:macro reference target not found: PyMODINIT_FUNC.

I will fix it.

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner force-pushed the doc_pyapi_func_no_link branch from cbd269f to 553fdd0 Compare

September 11, 2023 20:58

serhiy-storchaka reviewed

Sep 11, 2023

View reviewed changes

serhiy-storchaka left a comment

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.

PyMODINIT_FUNC is purposed to be used in the user code. It is even used in examples.

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner force-pushed the doc_pyapi_func_no_link branch from 553fdd0 to 5eba491 Compare

September 11, 2023 21:20

vstinner commented Sep 11, 2023

Copy link

Copy Markdown

Member Author

PyMODINIT_FUNC is purposed to be used in the user code. It is even used in examples.

Oh right.

I fixed my PR:

Document PyMODINIT_FUNC.
Remove links to PyAPI_FUNC() and PyAPI_DATA().

Please review it again.

@hugovk: I added "the" in the added doc ;-)

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner force-pushed the doc_pyapi_func_no_link branch from 5eba491 to 5847580 Compare

September 11, 2023 21:21

serhiy-storchaka reviewed

Sep 12, 2023

View reviewed changes

Comment thread

Doc/c-api/intro.rst Outdated

Comment on lines +111 to +112

		type is :c:expr:`PyObject*`. The macro should only be used in the Python C
		API.

serhiy-storchaka Sep 12, 2023

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.

"The macro should only be used in the Python C API." -- I think that it is wrong.

This description is not clear, and does not help. The use of PyMODINIT_FUNC is actually described in Doc/extending/extending.rst:

This structure, in turn, must be passed to the interpreter in the module's
initialization function.  The initialization function must be named
:c:func:`!PyInit_name`, where *name* is the name of the module, and should be the
only non-\ ``static`` item defined in the module file::

   PyMODINIT_FUNC
   PyInit_spam(void)
   {
       return PyModule_Create(&spammodule);
   }

Note that :c:macro:`PyMODINIT_FUNC` declares the function as ``PyObject *`` return type,
declares any special linkage declarations required by the platform, and for C++
declares the function as ``extern "C"``.

It would be better if PyMODINIT_FUNC links would refer to that text. Or at least make a reference to it from the declaration in Doc/c-api/intro.rst.

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner Sep 13, 2023

Copy link

Copy Markdown

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

Ok, I copied this doc to Doc/c-api/intro.rst.

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner commented Sep 13, 2023

Copy link

Copy Markdown

Member Author

@serhiy-storchaka: I completed PyMODINIT_FUNC doc. Would you mind to review my doc change again?

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner requested review from a team, 1st1, JelleZijlstra, brettcannon, ericsnowcurrently, ethanfurman, iritkatriel, lysnikolaou, markshannon, ncoghlan, pablogsal, rhettinger and warsaw as code owners

September 13, 2023 15:59

vstinner removed request for 1st1, JelleZijlstra, asvetlov, gvanrossum, iritkatriel, markshannon, warsaw and willingc

September 13, 2023 16:19

vstinner commented Sep 13, 2023

Copy link

Copy Markdown

Member Author

Oops sorry, I tested a Git tool and it messed up my PR :-( I fixed my PR.

All reactions

Sorry, something went wrong.

Uh oh!

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

serhiy-storchaka approved these changes

Sep 13, 2023

View reviewed changes

Comment thread

Doc/c-api/intro.rst Outdated

		declarations required by the platform, and for C++ declares the function as
		``extern "C"``.

		The initialization function must be named :c:func:`!PyInit_name`, where

serhiy-storchaka Sep 13, 2023

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.

Maybe write :samp:`PyInit_{name}`? It renders the whole name as a code, but name additionally is in italic, as a variable part.

Sorry, something went wrong.

Uh oh!

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

hugovk reacted with thumbs up emoji

All reactions

👍 1 reaction

vstinner Sep 14, 2023

Copy link

Copy Markdown

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

I never saw :samp:, let me try.

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner Sep 14, 2023

Copy link

Copy Markdown

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

I see, it looks good: https://cpython-previews--109236.org.readthedocs.build/en/109236/c-api/intro.html#c.PyMODINIT_FUNC

Sorry, something went wrong.

Uh oh!

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

All reactions


          Use :samp:

cc5da84

vstinner merged commit d7a27e5 into python:main

Sep 14, 2023

vstinner deleted the doc_pyapi_func_no_link branch

September 14, 2023 12:56

bedevere-app Bot removed the awaiting core review label

Sep 14, 2023

vstinner commented Sep 14, 2023

Copy link

Copy Markdown

Member Author

Merged, thanks for the review @serhiy-storchaka.

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented Sep 14, 2023

Copy link

Copy Markdown

Member Author

Merged, thanks for the review @serhiy-storchaka.

Oh, and thanks @hugovk for the review as well :-)

hugovk reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

vstinner added needs backport to 3.11

only security fixes

needs backport to 3.12 only security fixes labels

Sep 27, 2023

miss-islington commented Sep 27, 2023

Copy link

Copy Markdown

Contributor

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

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

miss-islington commented Sep 27, 2023

Copy link

Copy Markdown

Contributor

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

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

miss-islington commented Sep 27, 2023

Copy link

Copy Markdown

Contributor

Sorry, @vstinner, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker d7a27e527d7e669d2e45cff80ad725978226477c 3.11

All reactions

Sorry, something went wrong.

Uh oh!

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

miss-islington assigned vstinner

Sep 27, 2023

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Sep 27, 2023


          pythongh-107298: Document PyMODINIT_FUNC macro (pythonGH-109236)

2d07b75

Document PyMODINIT_FUNC macro.

Remove links to PyAPI_FUNC() and PyAPI_DATA() macros since they are
not documented. These macros should only be used to define the Python
C API. They should not be used outside Python code base.
(cherry picked from commit d7a27e5)

Co-authored-by: Victor Stinner <[email protected]>

bedevere-app Bot commented Sep 27, 2023

Copy link

Copy Markdown

GH-109947 is a backport of this pull request to the 3.12 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-app Bot removed the needs backport to 3.12 only security fixes label

Sep 27, 2023

bedevere-app Bot commented Sep 27, 2023

Copy link

Copy Markdown

GH-109948 is a backport of this pull request to the 3.11 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-app Bot removed the needs backport to 3.11 only security fixes label

Sep 27, 2023

vstinner mentioned this pull request

Sep 27, 2023

[3.12] gh-101100: Fix Sphinx warnings in Doc/using/configure.rst (GH-109931) #109937

Merged

vstinner added a commit that referenced this pull request

Sep 27, 2023


          [3.11] gh-107298: Document PyMODINIT_FUNC macro (#109236) (#109948)

d951aae

gh-107298: Document PyMODINIT_FUNC macro (#109236)

Document PyMODINIT_FUNC macro.

Remove links to PyAPI_FUNC() and PyAPI_DATA() macros since they are
not documented. These macros should only be used to define the Python
C API. They should not be used outside Python code base.

(cherry picked from commit d7a27e5)

Yhg1s pushed a commit that referenced this pull request

Sep 27, 2023


          [3.12] gh-107298: Document PyMODINIT_FUNC macro (GH-109236) (#109947)

fc4fddb

gh-107298: Document PyMODINIT_FUNC macro (GH-109236)

Document PyMODINIT_FUNC macro.

Remove links to PyAPI_FUNC() and PyAPI_DATA() macros since they are
not documented. These macros should only be used to define the Python
C API. They should not be used outside Python code base.
(cherry picked from commit d7a27e5)

Co-authored-by: Victor Stinner <[email protected]>

Reviewers

hugovk

hugovk approved these changes

serhiy-storchaka

serhiy-storchaka approved these changes

Uh oh!

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

Assignees

vstinner

Labels

docs

Documentation in the Doc dir

skip news

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

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.

GeezerVison

Conversation

vstinner commented Sep 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Sep 10, 2023

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Sep 11, 2023

Uh oh!

serhiy-storchaka Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner Sep 13, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner commented Sep 13, 2023

Uh oh!

vstinner commented Sep 13, 2023

Uh oh!

serhiy-storchaka Sep 13, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner commented Sep 14, 2023

Uh oh!

vstinner commented Sep 14, 2023

Uh oh!

miss-islington commented Sep 27, 2023

Uh oh!

miss-islington commented Sep 27, 2023

Uh oh!

miss-islington commented Sep 27, 2023

Uh oh!

bedevere-app Bot commented Sep 27, 2023

Uh oh!

bedevere-app Bot commented Sep 27, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vstinner commented Sep 10, 2023 •

edited

Loading

Uh oh!

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