Skip to content

cast: Add is_generic_type<T> #3857

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

EricCousineau-TRI
Copy link
Collaborator

Description

Adds explicit way to denote a is_generic_type<>, which is effectively bullet point (1) from the docs:
https://pybind11.readthedocs.io/en/stable/advanced/cast/index.html

Permits distinction from bullet points (2) and (3)

Suggested changelog entry:

Adds is_generic_type<> trait

@@ -36,6 +36,15 @@ class type_caster : public type_caster_base<type> {};
template <typename type>
using make_caster = type_caster<intrinsic_t<type>>;

template <typename T>
struct is_generic_type<T, enable_if_t<std::is_base_of<type_caster_generic, make_caster<T>>::value>>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From @rwgk in #2950

Two questions:

  1. Will this work if I add another specialization pair for is_base_of<smart_holder_type_caster_base_tag, ...>?

  2. What do you think about choosing a different name, to connect to the intended purpose instead of pegging the name on type_caster_generic, e.g. is_iterable_t_compatible? (Otherwise it will get really confusing that generic is connected to smart_holder_type_caster.)

(1) Yup! Can simply add the specialization and it should work out of the box.
(2) I know that was for prior PR; I believe this trait is more agnostic. See PR overview for rationale.

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Approving this change although I have a some uncertainties.

  • Could you please say something about the "why" in the PR description? Is the static_assert in isinstance the main motivation? What accidents does that prevent?

  • The naming seems to refer back to class generic_type. Is that a correct understanding? Would it make sense to add a comment to explain? — Pegging is_generic_type on the type_caster seems like "circumstantial evidence" (as opposed to direct evidence). Is a direct is_generic_type too difficult to implement? My guess: yes, but it would be good to explain (tersely is fine).

  • class generic_type is in pybind11.h

  • A struct is_generic_type forward declaration is in pytypes.h

  • The is_generic_type specializations are in cast.h

This makes me wonder: is the isinstance overload for detail::isinstance_generic in the right place? Could it be moved to cast.h?

I'm OK if the answer is no, the current pybind11 code organization has me wishing a lot and fixing that is obviously a bigger project. But if moving the isinstance overload is an option, that might help at least a little bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants