bpo-17045: Improve C-API doc for PyTypeObject. by ericsnowcurrently · Pull Request #7413 · python/cpython

ericsnowcurrently

Conversation

This is a quick port to git of a patch I wrote for the issue several years ago. I'm sure I have it mostly okay, but haven't had time yet to check it closely.

https://bugs.python.org/issue17045

		PyTypeObject Slot [#slots]_ Inherited [#inh]_ Type
		=========================================== ================= ======================
		:c:member:`~PyTypeObject.tp_name` .. char *
		:c:member:`~PyTypeObject.tp_basicsize` X int

Choose a reason for hiding this comment

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

Py_ssize_t

Choose a reason for hiding this comment

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

fixed

		=========================================== ================= ======================
		PyTypeObject Slot [#slots]_ Inherited [#inh]_ Type
		=========================================== ================= ======================
		:c:member:`~PyTypeObject.tp_name` .. char *

Choose a reason for hiding this comment

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

const char *tp_name

Choose a reason for hiding this comment

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

fixed

		:c:member:`~PyTypeObject.tp_setattro` X :c:type:`setattrofunc`
		:c:member:`~PyTypeObject.tp_as_buffer` \* PyBufferProcs *
		:c:member:`~PyTypeObject.tp_flags` ? long
		:c:member:`~PyTypeObject.tp_doc` .. char *

Choose a reason for hiding this comment

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

const char *

Choose a reason for hiding this comment

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

fixed

		=========================================== ================= ======================
		:c:member:`~PyTypeObject.tp_name` .. char *
		:c:member:`~PyTypeObject.tp_basicsize` X int
		:c:member:`~PyTypeObject.tp_itemsize` ? int

Choose a reason for hiding this comment

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

Py_ssize_t

Choose a reason for hiding this comment

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

fixed

		:c:member:`~PyTypeObject.tp_getattro` X :c:type:`getattrofunc`
		:c:member:`~PyTypeObject.tp_setattro` X :c:type:`setattrofunc`
		:c:member:`~PyTypeObject.tp_as_buffer` \* PyBufferProcs *
		:c:member:`~PyTypeObject.tp_flags` ? long

Choose a reason for hiding this comment

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

unsigned long

Choose a reason for hiding this comment

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

fixed


		.. XXX belongs elsewhere

		Docs for PyGetSetDef::

Choose a reason for hiding this comment

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

Described in structures.rst.

Choose a reason for hiding this comment

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

fixed


		Default:

		PyBaseObject_Type provides a tp_richcompare. However, if only

Choose a reason for hiding this comment

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

Add markup for PyBaseObject_Type and tp_richcompare.

Choose a reason for hiding this comment

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

fixed

		Default:

		This slot has no default. For static types, if the field is
		NULL then no __dict__ gets created for instances.

Choose a reason for hiding this comment

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

Add markup for NULL and __dict__.

Choose a reason for hiding this comment

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

fixed

		:c:func:`PyType_GenericAlloc`, to force a standard heap
		allocation strategy.

		For static subtypess, PyBaseObject_Type uses

Choose a reason for hiding this comment

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

Add markup for PyBaseObject_Type.

Choose a reason for hiding this comment

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

Also, subtypess -> subtypes

Choose a reason for hiding this comment

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

typo fixed

Choose a reason for hiding this comment

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

fixed

		Default:

		For static types this field has no default. This means if the
		slot is defined as NULL, the type cannot be called to create new

Choose a reason for hiding this comment

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

Add markup for NULL.

Choose a reason for hiding this comment

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

fixed

I'm not sure about usefulness of Default: notes. If the slot is inherited, then the default is the parent's value. It looks to me that Inheritane: notes cover this, and Default: notes don't add new information.

If I recall correctly, there are a few slots with defaults. Would it be better to omit the "default" section in the rest of the cases? FWIW, I added the "default" section because the defaults aren't always clear.

Ah, I need to run "make suspicious". :/

		A slot name in parentheses indicates it is (effectively) deprecated.
		Names in angle brackets should be treated as read-only.
		Names in square brackets are for internal use only.
		An asterisk means the field must be non-NULL.

Choose a reason for hiding this comment

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

Can you find another notation that doesn’t look like a pointer type or dereferencing a pointer?

Choose a reason for hiding this comment

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

Sure. I was following the existing convention in the file, but agree it's less clear.

Choose a reason for hiding this comment

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

Oh, you're talking about the notation in the table. :) I'll change it to something else.


		.. code-block:: none

		X - PyType_Ready always sets this value (if NULL)

Choose a reason for hiding this comment

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

The always and brackets confuse me. Would this be more correct: “PyTypeReady sets this value if it is NULL”?

Choose a reason for hiding this comment

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

I agree that it's not as clear as I want. My goal was to emphasize that it does get set by PyType_Ready(). I'll make it more clear.

		This field is inherited by subtypes together with :c:member:`~PyTypeObject.tp_setattr`: a subtype
		inherits both :c:member:`~PyTypeObject.tp_setattr` and :c:member:`~PyTypeObject.tp_setattro` from its base type when
		the subtype's :c:member:`~PyTypeObject.tp_setattr` and :c:member:`~PyTypeObject.tp_setattro` are both NULL.
		THE SUBTYpe's :c:member:`~PyTypeObject.tp_setattr` and :c:member:`~PyTypeObject.tp_setattro` are both NULL.

Choose a reason for hiding this comment

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

This line looks like a mistake

Choose a reason for hiding this comment

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

Yep. I'll fix it.


		Default:

		:c:type:`PyBaseObject_Type` provides a :attr:`tp_richcompare`.

Choose a reason for hiding this comment

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

Drop a or add a real noun, i.e. “provides tp_richcompare” or “provides a tp_richcompare implementation”

Choose a reason for hiding this comment

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

agreed

I've addressed all review comments.

bpo-17045: Improve C-API doc for PyTypeObject. by ericsnowcurrently · Pull Request #7413 · python/cpython

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels