Skip to content
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

Separate optional and variant support from other STL containers (#1207) #1210

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Separate optional and variant support from other STL containers (#1207) #1210

wants to merge 3 commits into from

Conversation

funchal
Copy link

@funchal funchal commented Dec 8, 2017

Move std::optional and std::variant conversion support, which were previously in pybind11/stl.h, to a new header file pybind11/utility.h since they are in the STL utility library in C++17. Include pybind11/utility.h from pybind11/stl.h so that we maintain full backward compatibility. Update the docs, tests and changelog to mention the new header.

@funchal funchal changed the title Separate optional and variant support from other STL containers #1207 Separate optional and variant support from other STL containers (#1207) Dec 8, 2017
@jagerman
Copy link
Member

jagerman commented Dec 8, 2017

This rename sounds good to me in principle, but I have one concern: utility.h is a pretty generic name; it would be nice to attach the notion of this being stl <utility> support as opposed to some sort of pybind11 utilities.

Thoughts (to you and @wjakob and anyone else who wants to offer an opinion on the matter) on going a bit bigger on this reorganization by putting this into pybind11/stl/utility.h, moving the container support into something like stl/containers.h, and the container binding from stl_bind.h into pybind11/stl/bind.h. pybind11/stl.h could then be a meta-header that just includes all the various pybind11/stl/*.h feature headers, and pybind11/stl_bind.h would become a deprecated include for pybind11/stl/bind.h.

We already added the detail directory in 2.2, and the plan in #708 (and related in #864) will almost certainly reorganize by introducing other directories.

@funchal
Copy link
Author

funchal commented Dec 16, 2017

@jagerman Thanks for having a look at this. I thought about your comment for a bit, and here's my opinion for what it's worth:

  • There's a reason why pybind11/stl.h isn't included automatically by pybind11.h, as it introduces possibly unwanted (and perhaps even arguably confusing) container conversion behavior. In some sense, pybind11/stl_bind.h is an alternative to pybind11/stl.h that uses api binds instead of conversion. We should be careful about keeping this distinction clear, if/when we go ahead with the header re-organisation you proposed. Perhaps bind/stl.h and cast/stl.h?
  • I might be wrong, but I feel like std::variant and std::optional are in a bit of a different situation. Similarly to std::tuple/std::pair etc, their behavior appears to be totally fine imho, and perhaps they should just be included automatically by pybind11.h, like std::tuple/std::pair.
  • I agree with the reorganization but there are diminishing returns. In particular, I believe that reorganizing api headers (those that enable different combinations of features) is way more important than reorganizing detail internals. It also seems more important than breaking down headers into various individual feature headers pybind11/stl/*.h etc as you also suggested, since I find it unlikely that these would be individually included (most users would just use the meta-header).

@jagerman
Copy link
Member

There's a reason why pybind11/stl.h isn't included automatically by pybind11.h, as it introduces possibly unwanted (and perhaps even arguably confusing) container conversion behavior.

The main reason is, I believe, just to keep the compilation a bit faster but not pulling in extra headers needed for stl containers in cases where they aren't needed. Including doesn't exactly change the behaviour: without it, trying to return a stl container just results in a failure because pybind won't know how the cast the type to Python (because it'll get to the generic type caster, but std::vector<Foo> is not a registered type). It's not clear to me that the non-stl.h behaviour is ever particular useful.

I might be wrong, but I feel like std::variant and std::optional are in a bit of a different situation.

I agree with you that variant and optional are essentially equivalent in their level of purpose to tuple and pair. Those, however, are in cast.h sort of by accident: the function argument dispatcher used to use a std::tuple, with special methods in the tuple caster specific to loading argument lists. That's no longer the case (as of 2.0, IIRC)—argument loading now goes via the detail::argument_loader class which is entirely separate. So if std::tuple support were being added today, it might well be in stl.h (and std::pair goes along with std::tuple--as of 2.2 they are handled by the same code).

I agree with the reorganization but there are diminishing returns. In particular, I believe that reorganizing api headers (those that enable different combinations of features) is way more important than reorganizing detail internals.

Agreed. That's basically why we moved things into detail/ for 2.2, but not other subdirectories. Partly whether this happens depends on whether we do the caster overhaul for 2.3, which will benefit from it.

@YannickJadoul
Copy link
Collaborator

Adding this to finally be resolved in 2.7.0 :-) Maybe through a new, non-conflicting PR, though.

@YannickJadoul YannickJadoul added this to the v2.7 milestone Dec 24, 2020
@henryiii henryiii modified the milestones: v2.7, v2.8 Jul 16, 2021
@henryiii henryiii modified the milestones: v2.8, v2.9 Oct 4, 2021
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.

4 participants