FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add a page on changing the C API (PEP-652) by encukou · Pull Request #682 · python/devguide · GitHub

  1. python
  2. devguide

Repository navigation

Add a page on changing the C API (PEP-652) - #682

Merged
encukou merged 3 commits into
python:masterfrom
encukou:c-api
May 4, 2021
ConversationCommits (3)ChecksFiles changed
Merged

Add a page on changing the C API (PEP-652)#682
encukou merged 3 commits into
python:masterfrom
encukou:c-api

Conversation

encukou commented Apr 29, 2021

Copy link
Copy Markdown
Member

This repeats points currently said in Include/README.rst.
I intend to replace most of that README with a link to this document.

The main change from that document are:

  • The three "rings" are not defined by where things are declared.
    It would be good to have them in the right place, but it's
    not yet the case, and for technical reasons we might never reach
    the ideal. (In Include/README.rst it makes more sense to make
    the locations more prominent.)
  • The order is switched. For changing the Limited API you generally
    need to read (and follow guidelines in) the preceding sections
    as well.

I added points from PEP-652 (Maintaining the Stable ABI).

Add a page on changing the C API
460c430 
This repeats points currently said in `Include/README.rst`.
I intend to replace most of that README with a link to this document.

The main change from that document are:
- The three "rings" are not defined by where things are declared.
  It would be good to have them in the right place, but it's
  not yet the case, and for technical reasons we might never reach
  the ideal. (In `Include/README.rst` it makes more sense to make
  the locations more prominent.)
- The order is switched. For changing the Limited API you generally
  need to read (and follow guidelines in) the preceding sections
  as well.

I added points from PEP 652 (Maintaining the Stable ABI).
encukou requested a review from vstinner April 29, 2021 14:49

encukou commented Apr 29, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

The page, rendered on RTD: https://cpython-devguide--682.org.readthedocs.build/c-api/

@erlend-aasland, you wrote Include/README.rst. Does this PR look OK to you?

Note that I also want to improve Doc/c-api/stable.rst: python/cpython#25668

encukou added a commit to encukou/cpython that referenced this pull request Apr 29, 2021
Replace Include/README.rst with a link to the devguide
15cc273 
Devguide PR: python/devguide#682
encukou mentioned this pull request Apr 29, 2021
Merged
erlend-aasland reviewed Apr 30, 2021
View reviewed changes

erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, with some nitpicks :)

Comment thread c-api.rst Outdated
Comment thread c-api.rst Outdated

- Please start a public discussion before expanding the Limited API

- The limited API and its intended use must follow standard C, not just

erlend-aasland Apr 30, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
- The limited API and its intended use must follow standard C, not just
- The Limited API and its intended use must follow standard C, not just

(Nit: I find the "its intended use" part a tiny bit weird. Can it be reformulated?)

erlend-aasland Apr 30, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Also, the "follow standard C" part is covered in line 130.

Comment thread c-api.rst Outdated
Comment thread c-api.rst Outdated
Comment thread c-api.rst
1. The internal, private API, available with ``Py_BUILD_CORE`` defined.
Ideally declared in ``Include/internal/``. Any API named with a leading
underscore is also considered private.
2. The public C API, available when ``Python.h`` is included normally.

erlend-aasland Apr 30, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe drop "normally"?

encukou May 4, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

By normally I mean without defining macros to select the other variants. I'll expand that in the section below.

encukou commented May 4, 2021

Copy link
Copy Markdown
Member Author

Thanks for the comments!
Do things look better now?

erlend-aasland approved these changes May 4, 2021
View reviewed changes

erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM :)

encukou merged commit 9b6b939 into python:master May 4, 2021
encukou deleted the c-api branch May 4, 2021 15:21

encukou commented May 4, 2021

Copy link
Copy Markdown
Member Author

Thanks!

vstinner commented May 26, 2021

Copy link
Copy Markdown
Member

https://devguide.python.org/c-api/ is great, thanks @encukou!

Next step: document C API removal ;-)

AA-Turner pushed a commit to AA-Turner/devguide that referenced this pull request Jun 17, 2022
Add a page on changing the C API (PEP-652) (pythonGH-682)
f3e0f88 
This repeats points currently said in `Include/README.rst`.
I intend to replace most of that README with a link to this document.

The main change from that document are:
- The three "rings" are not defined by where things are declared.
  It would be good to have them in the right place, but it's
  not yet the case, and for technical reasons we might never reach
  the ideal. (In `Include/README.rst` it makes more sense to make
  the locations more prominent.)
- The order is switched. For changing the Limited API you generally
  need to read (and follow guidelines in) the preceding sections
  as well.

On top of that, I addes points from PEP 652 (Maintaining the Stable ABI).


Co-Authored-By: Erlend Egeberg Aasland <erlend.aasland@innova.no>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

erlend-aasland erlend-aasland approved these changes
vstinner Awaiting requested review from vstinner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL