Fix stl_bind to support movable, non-copyable value types

[jagerman]

Fixes #487 (but not ready for merging).

We require copyable types for pretty much all vector modifiers, so this basically moves them all into a method that is only enabled for copyable types. (Technically we could allow pop and __delitem__ without copying, but it seems a little weird to expose deletion but no other modifiers).

In practice, this means that if the type is non-copyable, it will be, for all intents and purposes, readonly from the Python side--but at least it's something (currently it fails to compile).

This is still incomplete: it needs similar support for bind_map, and needs a better solution for the current dirty hack in cast.h, on which I'd appreciate input on how to deal with it. I definitely don't think it should remain as is: this was just a hack to get it working as a starting point.

The issue is that make_copy_constructor fails for std::vector<vt> where vt is non-copyable. The decltype(new std::vector<vt>(...)) succeeds (i.e. is a type), but an actual attempt to invoke the constructor results in a compilation failure. (std::is_copy_constructible also fails in the same way, i.e. by returning true even when actually invoking the copy constructor results in a compilation failure). For now I'm just hacking around it (and the hack is only on the non-MSVC path: the MSVC build of this will fail), but a better solution is needed. @AlZie (in #487) points to a workaround in http://stackoverflow.com/questions/18404108/issue-with-is-copy-constructible; any thoughts on something better?

[wjakob]

Thanks, that's a good catch. I think I prefer the approach from stackoverflow which defines a custom version of is_copy_constructible (perhaps in the pybind11::detail namespace?). This could then be partially specialized in stl.h. What do you think?

[jagerman]

ITYM stl_bind.h, but yes, that seems reasonable. (A vector with a read-only type is going to be completely useless with stl.h because we can't copy it).

[jagerman]

Added the same support for non-copyable map types (and rewrote the commit history to squash and split up commits).

I think I prefer the approach from stackoverflow which defines a custom version of is_copy_constructible (perhaps in the pybind11::detail namespace?). This could then be partially specialized in stl.h. What do you think?

That's basically the approach here, except the specialization is right along with the general case, using one specialization that should work for any stl container types.

Putting the override in stl.h won't work (stl_bind.h doesn't depend on it), and putting it in stl_bind.h won't work, either, because we don't actually know what the container type is: it's just a template argument given to the bind_vector or bind_map function.

We also can't just list it for all stl types, because there are stl types like std::deque which are perfectly usable with bind_vector, but are never #included anywhere in pybind11.

The implementation in this PR seems to have a pretty negligible cost, though (comparing to current master):

------ pybind11_tests.cpython-35m-x86_64-linux-gnu.so file size: 1007120 (change of +168 bytes = +0.02%)

... and now I'll go fix the build failure :)

[jagerman]

Fixed and squashed.

[wjakob]

A thought while reading through the diff -- some of this may also be related to the prior implementation:

I wonder if the vector getitem for the is_copy_constructible case makes sense: it always creates a copy. This could be undesirable if the value_type is a heap-allocated type bound with class_. I think the corresponding iter function uses an internal reference, which seems strangely inconsistent.

[wjakob]

Nifty :)

[jagerman]

Sounds good to me. One problem, though: making them both return by reference almost works, but breaks the std::vector<bool> case (because std::vector<bool>'s operator[] and iterators return a proxy object rather than a bool &). So since we need both implementations anyway, I'll keep the return-by-copy one enabled for arithmetic types, and return anything else by reference_internal. I'll also change the copying iterator to use ::copy instead of ::reference_internal.

[wjakob]

Sorry, one more change request that I didn't realize before: there duplication between cases which return a reference & use the return value policy internal_reference for non-arithmetic types, and return by value for arithmetic types.

I think that you can always use the first case even for arithmetic types. In that case, an arithmetic py::internal::type_caster partial overload is used, which doesn't care at all about return value policies.

This will allow to remove some duplication & make for easier reading.

[jagerman]

I think that you can always use the first case even for arithmetic types.

Unfortunately not for bool: std::vector<bool>'s iterators and operator[] don't actually return a bool reference, so it needs a special case. I could cut it down to just one specialization for std::vector<bool>, but we still need the pair.

The two different map versions, on the other hand, can be eliminated.

[jagerman]

Updated to use references whenever possible; the special copying case for vectors is now only relied on if Vector has an operator[] that returns something other than value_type &, which is only going to happen for std::vector<bool> (though could, in theory, also happen for a custom stl-like container class).

[wjakob]

LGTM, thanks!