Add `custom_type_setup` attribute #3287

jbms · 2021-09-20T20:51:19Z

This allows for custom modifications to the PyHeapTypeObject prior to
calling PyType_Ready. This may be used, for example, to define
tp_traverse and tp_clear functions.

Description

Suggested changelog entry:

``pybind11::custom_type_setup`` was added, for customizing the ``PyHeapTypeObject`` corresponding to a class, which may be useful for enabling garbage collection support, among other things.

rwgk

Initially the suggested changelog entry made me wonder: does custom_type_setup exist already? Is this PR enhancing the functionality?
But no, it's new.
How about:

pybind11::custom_type_setup was added, for customizing the PyHeapTypeObject corresponding to a class_, which may be useful for enabling garbage collection support, among other things.

jbms · 2021-09-21T02:47:46Z

Thanks, I updated the "suggested changelog" per your suggestion.

An alternative is to add direct support for garbage collection, similar to the other protocols. But still I think custom_type_setup is useful as an escape hatch, so that users can implement any protocol they like without waiting for pybind11 to specifically support it.

include/pybind11/detail/function_view.h

tests/test_custom_type_setup.py

jbms · 2021-09-21T21:04:54Z

All tests pass now.

rwgk

I believe this is a useful addition, although I also wonder if this will lead to N almost correct implementations for adding GC support (in other repos), instead of 1 firmly correct implementation (in this repo). — On balance I'm coming down on the side of "let it grow and maybe clean up later" vs slipping into over-engineering.

docs/advanced/classes.rst

include/pybind11/detail/function_view.h

rwgk · 2021-09-21T21:06:37Z

include/pybind11/detail/function_view.h

+    }
+
+    R (*erased_fn_)(void *, Arg...) = nullptr;
+    const void *erased_obj_;


type_erased_obj_?
If that's correct, I'd definitely spend the extra few characters for clarity.

tests/test_custom_type_setup.py

jbms · 2021-09-22T16:56:43Z

It looks like all tests now pass --- the one failure was just a spurious one.

rwgk

I'd feel much more comfortable with this PR if the documentation had a pointer to tested code elsewhere, and if we either reuse std::function, or have measurements to show that the function_view optimization is worth the extra code.

But I think it'll be best to first let @henryiii and @Skylion007 weigh in. I'm OK with this PR as-is if it gets support from a majority.

include/pybind11/detail/function_view.h

docs/advanced/classes.rst

henryiii · 2021-09-22T17:23:51Z

I'd feel much more comfortable with this PR if the documentation had a pointer to tested code elsewhere

The current documentation does not do this. We probably should use include markers and test the documentation examples that way, but we don't, and I don't think we should change styles in the middle of an unrelated PR. We in general have to be careful changing any public interface, and this makes that a public interface, so the docs shouldn't bit-rot too fast.

include/pybind11/attr.h

include/pybind11/detail/function_view.h

jbms · 2021-09-23T19:01:57Z

Note: Tests are now failing on Windows, due to an unrelated warning, presumably due to a compiler upgrade on github actions.

henryiii · 2021-09-23T19:36:46Z

The last build on master compiled after this, with no errors. Also Appveyor also failed. Notice it's only 2.7 failing (yuck).

… from Python

include/pybind11/attr.h

jbms · 2021-09-23T19:42:44Z

This is the error I'm seeing on both AppVeyor and github actions:

C:\projects\pybind11\include\pybind11/embed.h(108): warning C4996: 'mbstowcs': This function or variable may be unsafe. Consider using mbstowcs_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [C:\projects\pybind11\tests\test_embed\test_embed.vcxproj]
  C:\Program Files (x86)\Windows Kits\10\Include\10.0.10240.0\ucrt\stdlib.h(920): note: see declaration of 'mbstowcs'
C:\projects\pybind11\include\pybind11/embed.h(112): warning C4996: 'mbstowcs': This function or variable may be unsafe. Consider using mbstowcs_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [C:\projects\pybind11\tests\test_embed\test_embed.vcxproj]
  C:\Program Files (x86)\Windows Kits\10\Include\10.0.10240.0\ucrt\stdlib.h(920): note: see declaration of 'mbstowcs'
  test_interpreter.cpp
C:\projects\pybind11\include\pybind11/embed.h(108): error C2220: warning treated as error - no 'object' file generated [C:\projects\pybind11\tests\test_embed\test_embed.vcxproj]
C:\projects\pybind11\include\pybind11/embed.h(108): warning C4996: 'mbstowcs': This function or variable may be unsafe. Consider using mbstowcs_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [C:\projects\pybind11\tests\test_embed\test_embed.vcxproj]
  C:\Program Files (x86)\Windows Kits\10\Include\10.0.10240.0\ucrt\stdlib.h(920): note: see declaration of 'mbstowcs'
C:\projects\pybind11\include\pybind11/embed.h(112): warning C4996: 'mbstowcs': This function or variable may be unsafe. Consider using mbstowcs_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [C:\projects\pybind11\tests\test_embed\test_embed.vcxproj]
  C:\Program Files (x86)\Windows Kits\10\Include\10.0.10240.0\ucrt\stdlib.h(920): note: see declaration of 'mbstowcs'

include/pybind11/attr.h

jbms · 2021-09-24T00:54:50Z

Okay, all tests are passing now.

docs/advanced/classes.rst

tests/test_custom_type_setup.cpp

include/pybind11/detail/function_view.h

This allows for custom modifications to the PyHeapTypeObject prior to calling `PyType_Ready`. This may be used, for example, to define `tp_traverse` and `tp_clear` functions.

rwgk

Looks good to me.

Potentially adding function_view in another PR sounds fine, although only if we can show that the extra code has some measurable benefit.

jbms · 2021-09-24T18:11:38Z

Is there anything left to do here?

rwgk · 2021-09-24T18:15:12Z

Is there anything left to do here?

@henryiii, @Skylion007, are you OK with this PR as it is now?

rwgk · 2021-09-24T19:08:14Z

Thanks @henryiii !

jbms requested a review from henryiii as a code owner September 20, 2021 20:51

jbms force-pushed the feat-custom-type-setup branch from 1cd90e3 to c3d0c0e Compare September 20, 2021 20:52

rwgk self-requested a review September 21, 2021 02:33

rwgk reviewed Sep 21, 2021

View reviewed changes

jbms force-pushed the feat-custom-type-setup branch 3 times, most recently from 8e0d428 to dc4fbfe Compare September 21, 2021 06:01

Skylion007 reviewed Sep 21, 2021

View reviewed changes

include/pybind11/detail/function_view.h Outdated Show resolved Hide resolved

Skylion007 reviewed Sep 21, 2021

View reviewed changes

include/pybind11/detail/function_view.h Outdated Show resolved Hide resolved

jbms force-pushed the feat-custom-type-setup branch from dc4fbfe to 480dd77 Compare September 21, 2021 17:14

Skylion007 reviewed Sep 21, 2021

View reviewed changes

include/pybind11/detail/function_view.h Outdated Show resolved Hide resolved

jbms force-pushed the feat-custom-type-setup branch from 480dd77 to 7dd511e Compare September 21, 2021 17:53

Skylion007 reviewed Sep 21, 2021

View reviewed changes

tests/test_custom_type_setup.py Outdated Show resolved Hide resolved

jbms force-pushed the feat-custom-type-setup branch 5 times, most recently from e295d8c to e06f7ca Compare September 21, 2021 20:17

rwgk reviewed Sep 21, 2021

View reviewed changes

jbms force-pushed the feat-custom-type-setup branch from e06f7ca to 6863368 Compare September 21, 2021 23:19

rwgk reviewed Sep 22, 2021

View reviewed changes

include/pybind11/detail/function_view.h Outdated Show resolved Hide resolved

docs/advanced/classes.rst Show resolved Hide resolved

rwgk reviewed Sep 23, 2021

View reviewed changes

include/pybind11/attr.h Outdated Show resolved Hide resolved

include/pybind11/detail/function_view.h Outdated Show resolved Hide resolved

jbms force-pushed the feat-custom-type-setup branch 2 times, most recently from cdfbcf8 to f808a98 Compare September 23, 2021 18:53

Fix pybind11::object::operator= to be safe if *this is accessible…

5025e0e

… from Python

henryiii reviewed Sep 23, 2021

View reviewed changes

include/pybind11/attr.h Outdated Show resolved Hide resolved

jbms closed this Sep 23, 2021

jbms reopened this Sep 23, 2021

jbms force-pushed the feat-custom-type-setup branch from 8df806b to de5147b Compare September 23, 2021 19:43

henryiii reviewed Sep 23, 2021

View reviewed changes

include/pybind11/attr.h Outdated Show resolved Hide resolved

jbms force-pushed the feat-custom-type-setup branch from de5147b to a0145fa Compare September 23, 2021 22:13

rwgk reviewed Sep 24, 2021

View reviewed changes

docs/advanced/classes.rst Outdated Show resolved Hide resolved

jbms force-pushed the feat-custom-type-setup branch from a0145fa to 2d4e528 Compare September 24, 2021 01:13

Skylion007 reviewed Sep 24, 2021

View reviewed changes

tests/test_custom_type_setup.cpp Outdated Show resolved Hide resolved

include/pybind11/detail/function_view.h Outdated Show resolved Hide resolved

Add custom_type_setup attribute

74af2d8

This allows for custom modifications to the PyHeapTypeObject prior to calling `PyType_Ready`. This may be used, for example, to define `tp_traverse` and `tp_clear` functions.

jbms force-pushed the feat-custom-type-setup branch from 2d4e528 to 74af2d8 Compare September 24, 2021 01:29

rwgk approved these changes Sep 24, 2021

View reviewed changes

henryiii approved these changes Sep 24, 2021

View reviewed changes

rwgk merged commit 62c4909 into pybind:master Sep 24, 2021

github-actions bot added the needs changelog Possibly needs a changelog entry label Sep 24, 2021

henryiii removed the needs changelog Possibly needs a changelog entry label Sep 24, 2021

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#3287

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `custom_type_setup` attribute #3287

Add `custom_type_setup` attribute #3287

jbms commented Sep 20, 2021 •

edited by henryiii

Loading

rwgk left a comment

jbms commented Sep 21, 2021 •

edited

Loading

jbms commented Sep 21, 2021

rwgk left a comment

rwgk Sep 21, 2021

jbms Sep 21, 2021

jbms commented Sep 22, 2021

rwgk left a comment

henryiii commented Sep 22, 2021

jbms commented Sep 23, 2021

henryiii commented Sep 23, 2021 •

edited

Loading

jbms commented Sep 23, 2021

jbms commented Sep 24, 2021

rwgk left a comment

jbms commented Sep 24, 2021

rwgk commented Sep 24, 2021

rwgk commented Sep 24, 2021

Add custom_type_setup attribute #3287

Add custom_type_setup attribute #3287

Conversation

jbms commented Sep 20, 2021 • edited by henryiii Loading

Description

Suggested changelog entry:

rwgk left a comment

Choose a reason for hiding this comment

jbms commented Sep 21, 2021 • edited Loading

jbms commented Sep 21, 2021

rwgk left a comment

Choose a reason for hiding this comment

rwgk Sep 21, 2021

Choose a reason for hiding this comment

jbms Sep 21, 2021

Choose a reason for hiding this comment

jbms commented Sep 22, 2021

rwgk left a comment

Choose a reason for hiding this comment

henryiii commented Sep 22, 2021

jbms commented Sep 23, 2021

henryiii commented Sep 23, 2021 • edited Loading

jbms commented Sep 23, 2021

jbms commented Sep 24, 2021

rwgk left a comment

Choose a reason for hiding this comment

jbms commented Sep 24, 2021

rwgk commented Sep 24, 2021

rwgk commented Sep 24, 2021

Add `custom_type_setup` attribute #3287

Add `custom_type_setup` attribute #3287

jbms commented Sep 20, 2021 •

edited by henryiii

Loading

jbms commented Sep 21, 2021 •

edited

Loading

henryiii commented Sep 23, 2021 •

edited

Loading