GeezerVison

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

GeezerVison is showing github.com — Add a page on changing the C API (PEP-652) by encukou · Pull Request #682 · python/devguide

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

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


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

the-knights-who-say-ni added the CLA signed label

Apr 29, 2021

encukou requested a review from vstinner

April 29, 2021 14:49


          Remove a reference that's not in CPython docs yet.

18edee8

encukou force-pushed the c-api branch from f98bc57 to 18edee8 Compare

April 29, 2021 14:53

encukou commented Apr 29, 2021 •

edited

Loading

Uh oh!

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

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

All reactions

Sorry, something went wrong.

Uh oh!

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

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

bpo-43795: PEP 652 user documentation python/cpython#25668

Merged

erlend-aasland reviewed

Apr 30, 2021

View reviewed changes

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.

LGTM, with some nitpicks :)

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

c-api.rst Resolved

Uh oh!

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

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

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

erlend-aasland Apr 30, 2021

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

c-api.rst Resolved

Uh oh!

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

Comment thread

c-api.rst Resolved

Uh oh!

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

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

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 drop "normally"?

Sorry, something went wrong.

Uh oh!

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

All reactions

encukou May 4, 2021

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.

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

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


          Address issues from review

be8adaa

encukou commented May 4, 2021

Copy link

Copy Markdown

Member Author

Thanks for the comments!
Do things look better now?

erlend-aasland 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.

erlend-aasland approved these changes

May 4, 2021

View reviewed changes

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.

LGTM :)

Sorry, something went wrong.

Uh oh!

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

All reactions

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!

All reactions

Sorry, something went wrong.

Uh oh!

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

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

erlend-aasland reacted with laugh emoji

All reactions

😄 1 reaction

Sorry, something went wrong.

Uh oh!

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

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 <[email protected]>

Reviewers

erlend-aasland

erlend-aasland approved these changes

vstinner

Awaiting requested review from vstinner

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

4 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

encukou commented Apr 29, 2021

Uh oh!

encukou commented Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erlend-aasland Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

erlend-aasland Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

erlend-aasland Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

encukou May 4, 2021

Choose a reason for hiding this comment

Uh oh!

encukou commented May 4, 2021

Uh oh!

erlend-aasland left a comment

Choose a reason for hiding this comment

Uh oh!

encukou commented May 4, 2021

Uh oh!

vstinner commented May 26, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

encukou commented Apr 29, 2021 •

edited

Loading

Uh oh!

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