bpo-33201: Modernize "Extension types" doc #6337

pitrou · 2018-04-01T19:18:15Z

Use C99 idioms for struct initialization
Streamline and clarify explanations
Fix some outdated code
Split the tutorial part from the other topics, for clarity

https://bugs.python.org/issue33201

pitrou · 2018-04-01T20:20:58Z

vadmium · 2018-04-02T01:51:50Z

Doc/extending/newtypes_tutorial.rst

+.. note::
+   The explicit cast to ``destructor`` above is needed because we defined
+   ``Noddy_dealloc`` to take a ``NoddyObject *`` argument, but the ``tp_dealloc``
+   function pointer expects to receive a ``PyObject *`` argument.  Otherwise,


Why not define Noddy_dealloc as accepting a PyObject pointer, and then convert the parameter to a NoddyObject pointer inside the function? That should avoid the compiler warning, and avoid Python calling the function with the wrong prototype, which I understand is undefined in standard C and Posix.

If you just want to explain the cast without rewriting the code, perhaps admit that the function prototype is wrong and the cast hides a related compiler warning.

It's an idiom that is safely used in many parts of CPython. I don't think it's "undefined" in C to do that (a pointer is a pointer, regardless of what it points to), unlike perhaps C++. Do you have a reference?

The alternative needs either many casts or using an additional variable inside the function, both of which are more cumbersome.

It's compllicated, so I could be wrong, but C99 says "If a converted pointer is used to call a function whose type is not compatible with the pointed-to type, the behavior is undefined" (sect. 6.3.2.3 par. 8). The function types are not compatible because "For two function types to be compatible, . . . corresponding parameters shall have compatible types" (sect. 6.7.5.3 par. 15). The parameters are not compatible because they point to incompatible structures (NoddyObject and PyObject): "For two pointer types to be compatible, . . . both shall be pointers to compatible types" (sect. 6.5.7.2 par. 2).

Yes, Python's own code does this function pointer casting a lot itself with success, but it never seemed right to me, and I don't think it is good to claim the cast is "needed" when there is an alternative.

There are a couple of recent bug reports about GCC 8 producing new warnings about explicit function pointer casts, e.g. https://bugs.python.org/issue33012 about METH_NOARGS and PyCFunction.

bpo-33012 is about wrong number of arguments, right?

I think you are right. Looking closer, the bug reports seem to be about GCC's "-Wcast-function-type" warning, and one of the documented exceptions to that warning is "Any parameter of pointer-type matches any other pointer-type."

vadmium · 2018-04-02T01:53:01Z

Doc/extending/newtypes.rst



 .. _weakref-support:

 Weak Reference Support
 ----------------------

-One of the goals of Python's weak-reference implementation is to allow any type
+One of the goals of Python's weak reference implementation is to allow any type


Why this change? Isn’t it an implementation of weak-references, not a reference-implementation that is weak?

We don't use the "weak-reference" spelling anywhere else - they're always just "weak references".

Yes, there were some odd spellings in that file, including "weak-reference" and "garbage-collector".

vadmium · 2018-04-02T03:59:14Z

Doc/extending/newtypes_tutorial.rst

+       }
+       Py_INCREF(value);
+       Py_CLEAR(self->first);
+       self->first = value;


If you’re improving the logic here for replacing an attribute, is there a reason why it has to be different to the INCREF / DECREF dance already done in Noddy_init?

You're right, it would be simpler for the reader to use the same idiom.

ncoghlan

This looks like a nice improvement to me. I've made a bunch of comments inline, but none of them are specific to the changes - they're just a result of reading through this doc for the first time in a long time and considering how it could be made more approachable.

ncoghlan · 2018-04-02T06:26:04Z

Doc/extending/newtypes.rst



 .. _weakref-support:

 Weak Reference Support
 ----------------------

-One of the goals of Python's weak-reference implementation is to allow any type
+One of the goals of Python's weak reference implementation is to allow any type


We don't use the "weak-reference" spelling anywhere else - they're always just "weak references".

ncoghlan · 2018-04-02T06:28:43Z

Doc/extending/newtypes.rst

-the :file:`Objects` directory, then search the C source files for ``tp_`` plus
-the function you want (for example, ``tp_richcompare``).  You will find examples
-of the function you want to implement.
+get the :term:`CPython` source code.  Go to the :file:`Objects` directory,


Perhaps link to https://devguide.python.org/setup/#checkout here?

ncoghlan · 2018-04-02T06:33:00Z

Doc/extending/newtypes_tutorial.rst

+
+Python allows the writer of a C extension module to define new types that
+can be manipulated from Python code, much like the built-in :class:`str`
+and :class:`list` types.  This is not hard; the code for all extension types


I don't think we need the "This is not hard" comment here, as for folks that aren't experienced C programmers, it is going to be hard.

We may also want to repeat the cross-references to Cython/CFFI/etc, and perhaps point to https://packaging.python.org/guides/packaging-binary-extensions/#an-overview-of-binary-extensions for further discussion of the available alternatives.

Agreed. I'll remove that wording.

ncoghlan · 2018-04-02T06:41:10Z

Doc/extending/newtypes_tutorial.rst

+That's it!  All that remains is to build it; put the above code in a file called
+:file:`noddy.c` and ::
+
+   from distutils.core import setup, Extension


Using setuptools is likely to be more robust here, since it tends to be better at finding compilers, emits up to date package metadata, and can be used to build wheel archives in combination with bdist_wheel.

https://packaging.python.org/tutorials/installing-packages/#ensure-pip-setuptools-and-wheel-are-up-to-date is the appropriate reference if folks just want to build locally, while they'll want https://packaging.python.org/tutorials/distributing-packages/ if they plan to actually create an sdist and/or wheel file for their extension module.

Right, I should add a link to that external resource.

ncoghlan · 2018-04-02T06:46:53Z

Doc/extending/newtypes_tutorial.rst

+easily use the :class:`PyTypeObject` it needs. It can be difficult to share
+these :class:`PyTypeObject` structures between extension modules.
+
+In this example we will create a :class:`Shoddy` type that inherits from the


Seeing Shoddy as the example name makes me think "This is an example of a shoddy class implementation".

Perhaps it would be clearer if the examples were just Example and SubExample rather than Noddy and Shoddy?

Alternatively, since "the Example example class" can read strangely, we could rename the examples based on what they demonstrate:

ExtMinimal (replacing Noddy1)

ExtWithAttributes (replacing Noddy2)

ExtWithSetters (replacing Noddy3)

ExtWithGC (replacing Noddy4)

ExtWithParent (replacing Shoddy)

Where the Ext prefix is short for Extension type.

To be honest, the names "Noddy" and "Shoddy" look like a joke by the original author(s). Overall the examples are silly, but it's hard to come with better ones (that still fit in the tutorial's structure).

I don't like the ExtSomething names either. We probably need something simple yet evocative...

At least Shoddy could be renamed SubList, I think.

Yeah, I'm not especially fond of the Ext... names either.

I do like Minimal as the name for the first example, since it does strip class definition down to its essence (i.e. no state, no behaviour).

PublicAttrs would describe the second example, since it exposes the data attributes directly.

GetSetAttrs would describe the third example, since it hides the data attributes behind getter and setter functions.

CyclicRefs would describe the fourth example, since it handles the case where the object ends up referencing itself (directly or indirectly).

And then SubList for the list subclassing example.

I find it nice that the current 4 examples keep the same object name, since each builds on top of the previous one. It feels like a distraction for the object name to change each time. Perhaps call it Something?

"the Custom example class" and "the example Custom class" both read OK to me (whereas those were the phrases that put me off Example), and I think it sounds OK in "custom module" and "Custom instance" as well.

So if it sounds OK to you as well, I'd suggest going with that.

I think Noddy and Shoddy are pretty good names for examples. Let not kill jokes in the documentation for a programming language named after the comic group.

I don't know if those names are jokes (what does "Noddy" mean exactly)? Those names are a bit confusing.

While Noddy and Shoddy are presumably meant just in fun, I found myself distracted by their actual meanings while reviewing Antoine's changes:

Noddy = this is pointless ("Yeah, it's a bit of a noddy change, but the boss insisted")
Shoddy = this is poorly implemented ("They did a shoddy job and I'm not happy with it")

For PEP 426, I used names like ComfyChair and SoftCushions in the examples, and picking some thematically appropriate arbitrary nouns from a Monty Python sketch may work here, too.

I think it's good to avoid confusion with bizarre names so will probably change to the more neutral "Custom".

serhiy-storchaka · 2018-04-03T18:03:41Z

Doc/includes/noddy.c

+    .tp_name = "noddy.Noddy",
+    .tp_doc = "Noddy objects",
+    .tp_basicsize = sizeof(NoddyObject),
+    .tp_itemsize = 0,


Is this needed?

The text explains tp_itemsize.

serhiy-storchaka · 2018-04-03T18:05:59Z

Doc/includes/noddy.c

-    Py_INCREF(&noddy_NoddyType);
-    PyModule_AddObject(m, "Noddy", (PyObject *)&noddy_NoddyType);
+    Py_INCREF(&NoddyType);
+    PyModule_AddObject(m, "Noddy", (PyObject *) &NoddyType);


Don't know if this is covered by PEP 7, but I think that it is more common to not add a space between closing ) and the casted value.

Hmm, I'd think the contrary.

serhiy-storchaka · 2018-04-03T18:11:49Z

Doc/includes/noddy2.c

-    (initproc)Noddy_init,      /* tp_init */
-    0,                         /* tp_alloc */
-    Noddy_new,                 /* tp_new */
+    .tp_name = "noddy.Noddy",


"noddy2.Noddy"?

Good catch :-)

serhiy-storchaka · 2018-04-03T18:18:01Z

Doc/includes/noddy3.c

    Py_INCREF(value);
+    Py_CLEAR(self->first);


You should use the dance with tmp here. Py_CLEAR(self->first) calls the destructor. The first attribute can be set directly in the destructor or it can be set in other thread when the GIL will be released in the destructor. self->first = value in the following line will rewrite the new value of the first attribute with leaking a reference.

Py_SETREF() could help here, but it is not in a public API.

Py_CLEAR should first transfer the pointer to a temporary before decref'ing it, so I don't see where the problem is.

The problem is virtually the same as in __init__.

Thread A calls Noddy_setfirst with the argument valueA, runs to Py_CLEAR(self->first) in which sets tmp = self->first; self->first = NULL and executes Py_XDECREF(tmp) which calls the destructor and releases the GIL. This causes switching to other thread.

Thread B calls Noddy_setfirst, and sets self->first to a new value valueB (it was set to NULL in thread A).

Thread A continues execution. The next line is self->first = valueA. It is expected that the value of self->first is NULL (as set by Py_CLEAR), but actually it is valueB. After setting self->first = valueA valueB will be leaked.

The same race condition is possible in a single-thread case if the destructor of the old attribute value calls Noddy_setfirst.

Thanks for clearing that up. I'll do the change.

serhiy-storchaka · 2018-04-03T18:25:53Z

Doc/includes/shoddy.c

 };

 PyMODINIT_FUNC
 PyInit_shoddy(void)
 {
-    PyObject *m;
-
+    PyObject* m;


It is more common to add a space before * than after it.

Uh, right. I'm confused between the coding styles of two different projects :-(

serhiy-storchaka · 2018-04-03T19:01:09Z

Doc/extending/newtypes_tutorial.rst

+
+This is what a Noddy object will contain.  ``PyObject_HEAD`` is mandatory
+at the start of each object struct and defines a field called ``ob_base``
+of type :c:type:`PyObject`, containing a pointer to a type object and a


In other order: a reference count and a type object.

serhiy-storchaka · 2018-04-03T19:07:46Z

Doc/extending/newtypes_tutorial.rst

+   >>> "" + noddy.new_noddy()
+   Traceback (most recent call last):
+     File "<stdin>", line 1, in <module>
+   TypeError: cannot add type "noddy.Noddy" to string


TypeError: can only concatenate str (not "noddy.Noddy") to str

Ha, thank you.

serhiy-storchaka · 2018-04-03T19:15:05Z

Doc/extending/newtypes_tutorial.rst

+This adds the type to the module dictionary.  This allows us to create
+:class:`Noddy` instances by calling the :class:`Noddy` class::
+
+   >>> import noddy


Is there needed a special directive for highlighting this code block as Python session rather of C code?

Hmm, you're right.

serhiy-storchaka · 2018-04-03T19:31:21Z

Doc/extending/newtypes_tutorial.rst

+   in C!
+
+We want to make sure that the first and last names are initialized to empty
+strings, so we provide a ``tp_new`` method::


I think that the original "a new method" means literally a new C function, not a method named "new" or something like. "provide a tp_new method" conflicts with a phrase following the code: "and install it in the tp_new member".

No, it was present in several places, and was really meaning "a tp_new method".

In any case "a tp_new method" looks wrong and confusing to me.

.. we provide a tp_new method ... and install it in the tp_new member.

tp_new is not a method, it is a PyTypeObject member (or slot). I think the original text is a tiny bit more correct. Look at wordings for other slots below.

You're right. I will use the more generic "handler".

serhiy-storchaka · 2018-04-03T19:33:36Z

Doc/extending/newtypes_tutorial.rst

+It is not required to define a ``tp_new`` member, and indeed many extension
+types will simply reuse ``PyType_GenericNew`` as done in the first version
+of the ``Noddy`` type above.  In this case, we use the ``tp_new`` method
+to first initialize the :attr:`first` and :attr:`last` attributes to


:c:member:`first` and :c:member:`last`

Hmm, right. That won't make much of a difference since this thing isn't documented...

serhiy-storchaka · 2018-04-03T20:07:57Z

Doc/extending/newtypes_tutorial.rst

+       {NULL}  /* Sentinel */
+   };
+
+(note that we used the :const:`METH_NOARGS` flag to indicate that the method


This hanging comment in parenthesis looks weird. What if move it up?

Now that we've defined the method, we need to create an array of method definitions (note that we used the :const:`METH_NOARGS` flag to indicate that the method is expecting no arguments other than *self*)::

Or keep it where it was in the original text?

Hmm, they look fine to me?

serhiy-storchaka · 2018-04-03T20:09:47Z

Doc/extending/newtypes_tutorial.rst

+   .tp_flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE,
+
+We rename :c:func:`PyInit_noddy` to :c:func:`PyInit_noddy2` and update the module
+name in the :c:type:`PyModuleDef` struct.


After changing the name of the module, update the full name of the class. They should be consistent.

serhiy-storchaka · 2018-04-03T20:26:04Z

Doc/extending/newtypes_tutorial.rst

+easily use the :class:`PyTypeObject` it needs. It can be difficult to share
+these :class:`PyTypeObject` structures between extension modules.
+
+In this example we will create a :class:`Shoddy` type that inherits from the


I think Noddy and Shoddy are pretty good names for examples. Let not kill jokes in the documentation for a programming language named after the comic group.

serhiy-storchaka · 2018-04-03T20:29:51Z

Doc/extending/newtypes_tutorial.rst

+create the memory for the object with its :c:member:`~PyTypeObject.tp_alloc`,
+but let the base class handle it by calling its own :c:member:`~PyTypeObject.tp_new`.
+
+When filling out the :c:func:`PyTypeObject` for the :class:`Shoddy` type, you see


Don't know what is more appropriate here, but perhaps not :c:func:.

Hmm, indeed :-)

serhiy-storchaka · 2018-04-03T20:30:44Z

Doc/extending/newtypes_tutorial.rst

+but let the base class handle it by calling its own :c:member:`~PyTypeObject.tp_new`.
+
+When filling out the :c:func:`PyTypeObject` for the :class:`Shoddy` type, you see
+a slot for :c:func:`tp_base`.  Due to cross-platform compiler issues, you can't


:c:member:

serhiy-storchaka · 2018-04-03T20:36:30Z

Doc/extending/newtypes.rst


   const char *tp_name; /* For printing */

-The name of the type - as mentioned in the last section, this will appear in
+The name of the type - as mentioned in the previous chapter, this will appear in


Needed longer dash.

serhiy-storchaka · 2018-04-03T20:37:49Z

Doc/extending/newtypes.rst

@@ -1035,7 +156,7 @@ example::
   static PyObject *
   newdatatype_repr(newdatatypeobject * obj)
   {
-       return PyUnicode_FromFormat("Repr-ified_newdatatype{{size:\%d}}",
+       return PyUnicode_FromFormat("Repr-ified_newdatatype{{size:%d}}",


Ha-ha, TeX artifact!

serhiy-storchaka · 2018-04-03T20:46:47Z

Doc/extending/newtypes.rst

+
+Any :term:`iterator` object should implement both :c:member:`~PyTypeObject.tp_iter`
+and :c:member:`~PyTypeObject.tp_iternext`.  An iterator's
+:c:member:`~PyTypeObject.tp_iter` handler should return a new reference


Maybe add a reference to PyObject_SelfIter?

serhiy-storchaka · 2018-04-03T20:51:09Z

Doc/extending/newtypes.rst

-   static void
-   instance_dealloc(PyInstanceObject *inst)
-   {
-       /* Allocate temporaries if needed, but do not begin


This comment is important. PyObject_ClearWeakRefs() should be called before calling any destructors.

Why is that?

I suppose that this is because destructors can resurrect the deallocated object (increment the refcount) if there are weak references to it. Following call of PyObject_ClearWeakRefs() will fail with raising SystemError because the refcount is not 0.

Interestingly, but not all builtin types follow this rule. Seems there are bugs.

…nize-extending

pitrou · 2018-04-06T18:34:57Z

I think I addressed most important comments now (including the renaming to "custom" and "sublist"). There are a couple stylistic comments that I chose not to follow.

serhiy-storchaka

What about the history? Doesn't mass renaming ruin it?

serhiy-storchaka · 2018-04-06T18:57:20Z

Doc/extending/newtypes_tutorial.rst


-   >>> "" + noddy.new_noddy()
+.. code-block:: python


"pycon" may be better option.

I didn't know about that.

Ok, it doesn't seem to make a difference compared to "python".

pitrou · 2018-04-06T20:46:37Z

What about the history? Doesn't mass renaming ruin it?

git is able to detect renames. You may need to use specific command-line options to benefit from it (e.g. git log --follow <filename>).

miss-islington · 2018-04-07T16:14:06Z

Thanks @pitrou for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

bedevere-bot · 2018-04-07T16:15:25Z

GH-6411 is a backport of this pull request to the 3.7 branch.

* bpo-33201: Modernize "Extension types" doc * Split tutorial and other topics * Some small fixes * Address some review comments * Rename noddy* to custom* and shoddy to sublist * Fix markup (cherry picked from commit 1d80a56) Co-authored-by: Antoine Pitrou <pitrou@free.fr>

miss-islington · 2018-04-07T16:16:09Z

Sorry, @pitrou, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 1d80a561734b9932961c546b0897405a3bfbf3e6 3.6

* bpo-33201: Modernize "Extension types" doc * Split tutorial and other topics * Some small fixes * Address some review comments * Rename noddy* to custom* and shoddy to sublist * Fix markup. (cherry picked from commit 1d80a56)

bedevere-bot · 2018-04-07T16:21:31Z

GH-6412 is a backport of this pull request to the 3.6 branch.

* bpo-33201: Modernize "Extension types" doc * Split tutorial and other topics * Some small fixes * Address some review comments * Rename noddy* to custom* and shoddy to sublist * Fix markup (cherry picked from commit 1d80a56) Co-authored-by: Antoine Pitrou <pitrou@free.fr>

* bpo-33201: Modernize "Extension types" doc * Split tutorial and other topics * Some small fixes * Address some review comments * Rename noddy* to custom* and shoddy to sublist * Fix markup. (cherry picked from commit 1d80a56)

pitrou added 2 commits April 1, 2018 21:03

bpo-33201: Modernize "Extension types" doc

84e9ecc

Split tutorial and other topics

2d5926d

the-knights-who-say-ni added the CLA signed label Apr 1, 2018

bedevere-bot added the awaiting merge label Apr 1, 2018

pitrou force-pushed the modernize-extending branch 2 times, most recently from 9a897d0 to 8302432 Compare April 1, 2018 20:17

Some small fixes

e2fa664

pitrou force-pushed the modernize-extending branch from 8302432 to e2fa664 Compare April 1, 2018 21:00

vadmium reviewed Apr 2, 2018

View reviewed changes

ncoghlan approved these changes Apr 2, 2018

View reviewed changes

serhiy-storchaka self-requested a review April 2, 2018 22:24

serhiy-storchaka reviewed Apr 3, 2018

View reviewed changes

pitrou added 3 commits April 6, 2018 19:14

Merge branch 'master' of https://github.com/python/cpython into moder…

7536112

…nize-extending

Address some review comments

073bc7a

Rename noddy* to custom* and shoddy to sublist

609b53d

pitrou added needs backport to 3.6 labels Apr 6, 2018

Fix markup

3b6a77f

serhiy-storchaka reviewed Apr 6, 2018

View reviewed changes

pitrou merged commit 1d80a56 into python:master Apr 7, 2018

bedevere-bot removed the awaiting merge label Apr 7, 2018

pitrou deleted the modernize-extending branch April 7, 2018 16:14

bedevere-bot removed the needs backport to 3.7 label Apr 7, 2018

bedevere-bot removed the needs backport to 3.6 label Apr 7, 2018

bpo-33201: Modernize "Extension types" doc #6337

bpo-33201: Modernize "Extension types" doc #6337

Conversation

pitrou commented Apr 1, 2018 • edited by bedevere-bot Loading

pitrou commented Apr 1, 2018

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

ncoghlan left a 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

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

pitrou commented Apr 6, 2018

serhiy-storchaka left a 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

pitrou commented Apr 6, 2018

miss-islington commented Apr 7, 2018

bedevere-bot commented Apr 7, 2018

pitrou commented Apr 1, 2018 •

edited by bedevere-bot

Loading