[WIP] Towards a solution for custom-type rvalue arguments #2047
Conversation
rhaschke
force-pushed
the
rvalue-argument-fix1
branch
from
fb0ecaa to
af9b355
Compare
|
Keeping the copy-only cast operator for |
|
Dear Robert, just a quick heads-up: I'm super busy with a conference deadline and don't have many free clock cycles in the next month. A quick bit of feedback is that this PR (and the sub-component #2048) seem very reasonable, but I'm scared by #2046 (both in terms of the heavy modifications and all of the things that could possibly go wrong). Moving a holder from Python to C++ and making it disappear in Python-land seems like a really counter-intuitive and even dangerous operation because the language provides no semantics for anything like it. I've also added @jagerman who authored some of the parts you are modifying. Best, |
|
Dear Wenzel, thanks for your feedback. I will be busy to the end of the month as well, I just used the holidays to look into pybind11 and migrate a project of mine from boost::python. I have to admit that I am new to template meta programming and I don't yet fully understand the architecture of pybind11. Particularly, I have the following questions currently:
Best, Robert |
rhaschke
force-pushed
the
rvalue-argument-fix1
branch
from
af9b355 to
04b0b99
Compare
|
Hi, |
|
It would be great if pybind11 would officially support this scenario which is common when wrapping modern C++ libs. |
|
The problem is that "moving" or "transferring ownership" is a concept that's not really known to Python (and might not result in the best Python API?). So I guess that's the reason there's a lot of hesitation, here as well as in other projects such as Boost.Python. However, it should (in principle) already be possible to do so, no? You could create a wrapper object (maybe even a class template), have the class/ |
Apologies. Apparently that's not possible, right now, and pybind11 already chokes when a function expects a rvalue reference. |
|
This does work for me, though, because you cán move from non-const lvalue references: #include <pybind11/pybind11.h>
#include <iostream>
namespace py = pybind11;
class X {
public:
X(int x) : m_x(x) { std::cout << "X::X(" << m_x << ")" << std::endl; }
X(const X&) = delete;
X(X &&other) : m_x(std::move(other.m_x)) { other.m_x = -1; std::cout << "X::X(X&&) (m_x = " << m_x << ")" << std::endl; }
X &operator=(const X&) = delete;
X &operator=(X&&) = default;
~X() { std::cout << "X::~X() (m_x = " << m_x << ")" << std::endl; }
private:
int m_x;
};
void sink(X &&x) { X(std::move(x)); };
PYBIND11_MODULE(example, m) {
py::class_<X>(m, "X");
m.def("source", [](int x) { return X(x); });
m.def("sink", [](X& x) { sink(std::move(x)); });
}>>> import example
>>> x = example.source(42)
X::X(42)
X::X(X&&) (m_x = 42)
X::~X() (m_x = -1)
>>> example.sink(x)
X::X(X&&) (m_x = 42)
X::~X() (m_x = 42)
>>> x = None
X::~X() (m_x = -1)I assume you could write a custom type caster checking if the |
|
|
|
Maybe on a more controversial note: there is always many wrong uses of a feature. But if they are good use cases (and I think there are, if you guys are not conviced I am willing to give more details), the library should support them. If not, then the user is left to code his or her own half-baked, non-standard workaround. |
True, yes, but there's a difference between changing an object and invalidating it? That's not exactly common in Python, no, an object that entirely invalid/untouchable? Another issue is that Python's variables work differently to C++. Every Python variable is basically already a shared pointer (with reference counting). So by "invalidating" one variable, you're also invalidating all other variables pointing to this same object. Yes, not insurmountable to explain, especially if it's can be checked and a nice error can be shown, but there's really a few places where C++'s memory management just doesn't fit with Python and vice versa :-/
I might agree here, though encouraging good design by default is still a good plan and will take away questions ;-) I'm not saying you're wrong, btw, or that this can never be part of pybind11; merely explaining why it's been a tricky thing to have in pybind11 (and one where a custom solution might be advisable, actually, since it'll often depend on the actual application on how to handle this?). But I'm curious about your use-case, yes. I found that often, there's a nice workaround that results in a more "pythonic" API. Btw, you're definitely not the first ones to come up with this problem. I know that for example @EricCousineau-TRI has played around with this before (see e.g., #1146, I believe? Lots of further discussion there, almost ancient history by now, for pybind11's lifetime). |
FWIW We're still using a flavor of #1146 in our fork: I can't say it was the worst decision to allow moving
For the case of |
|
To the point of this PR, though, it seems pretty straightforward at first blush. The net gain I see here is that users don't have to write odd wrappers to bind functions with rvalues; the fallout of move semantics does seem like it'd be the downstream dev's responsibility, which IMO are foot-guns that are still available in nominal pybind? (e.g. I'd be happy to review (albeit slowly), and see if we can try to create simulated edge cases that we could encode in unittests (e.g. if there ever is a point where a Python program is affected by move semantics). (I do think that we should de-scope supporting anything like |
|
Oh, upon reading #2040, it looks like the goal is to handle |
|
First, I am very happy to see that people are responding quickly to the comments, it's good to have an active community! I actually missed this line from @YannickJadoul
Well actually, a moved-from object is not invalid but in a "valid but unspecified state". So you can call any method that does not requires an invariant. E.g. you can call
Yes, but same thing if you are resizing an array to 0, this may be surprising to the other variables. I think this is conceptually the same usual kind of behavior
My use case is the following : I have a CFD problem where I am building the configuration step by step in Python. The classes describing the configuration itself are in C++. So let's say I want to build the initial solution fields inside a |
|
@BerengerBerthoul Since Yannick's workaround works for you, it seems like this PR would valuable to you (you're not expecting @rhaschke Would you be up for moving this forward w/o anything explicitly for |
Move semantics didn't exist when Boost.Python was written (2000-2003 timeframe). |
Right, of course! Thanks for that reminder. So yes, maybe this can be a project towards 2.7.0? Maybe we can make this safer with some opt-in flag, or so? |
|
@EricCousineau-TRI, I would be happy to move this forward, although, eventually, I would need support for moving unique_ptrs. |
My point about scoping is that this is irrelevant to |
For me, to decently review this, I'd need a few hours (after 2.6.0 is out, when I have some time) to fully get all the details of the type casters again. E.g., this first change of Apart from that, stealing objects from Python is rather tricky (given the way Python treats objects), so once there is something, I would like some input from @wjakob on this. Of course, once you allow objects to be stolen from pybind11, maybe there isn't a big difference anymore with stealing the holders. At the same time, stealing objects is already possible, since you can get a reference to the object? (not sure if that's an argument for or against this PR :-) ) I can see this can all be useful for power users and a bit of a gap between C++ and Python, but at the same time, I still don't believe it's a great Python API, so making this opt-in and slightly harder than necessary might push users towards creating more Python-like APIs. Any thoughts on that? |
Hi @YannickJadoul , I think the motivation behind this PR, and #2046, goes much deeper. See also my question #2583.
I actually started out wanting to eliminate the About disadvantages, I can see two: 1. an extra If I balance those disadvantages with the advantage of enabling wrapping of modern C++ code, I come out strongly on the "it's worth it" side, but I also think your "harder than necessary" idea is a good one. Maybe it could even be leveraged to address the 2. disadvantage above, via something like In the C++ bindings we'd need some mechanism to add the special |
Great. Thanks! I will apply those changes asap. |
include/pybind11/eigen.h
Outdated
| @@ -346,7 +346,8 @@ struct type_caster<Type, enable_if_t<is_eigen_dense_plain<Type>::value>> { | |||
|
|
|||
| operator Type*() { return &value; } | |||
| operator Type&() { return value; } | |||
| operator Type&&() && { return std::move(value); } | |||
| operator Type&&() { return std::move(value); } | |||
| operator Type() { return std::move(value); } | |||
There was a problem hiding this comment.
Does operator Type() here need the SFINAE treatement?
There was a problem hiding this comment.
The Eigen types always are move-constructible. Hence, no need for protection, I think.
This commit doesn't compile for me, neither with gcc nor with clang 😞
Shall I rebase onto master? |
See also: pybind#2583 Does not build with upstream master or pybind#2047, but builds with https://github.com/RobotLocomotion/pybind11 and almost runs: ``` Running tests in directory "/usr/local/google/home/rwgk/forked/EricCousineau-TRI/pybind11/tests": ================================================================================= test session starts ================================================================================= platform linux -- Python 3.8.5, pytest-5.4.3, py-1.9.0, pluggy-0.13.1 rootdir: /usr/local/google/home/rwgk/forked/EricCousineau-TRI/pybind11/tests, inifile: pytest.ini collected 2 items test_unique_ptr_member.py .F [100%] ====================================================================================== FAILURES ======================================================================================= _____________________________________________________________________________ test_pointee_and_ptr_owner ______________________________________________________________________________ def test_pointee_and_ptr_owner(): obj = m.pointee() assert obj.get_int() == 213 m.ptr_owner(obj) with pytest.raises(ValueError) as exc_info: > obj.get_int() E Failed: DID NOT RAISE <class 'ValueError'> test_unique_ptr_member.py:17: Failed ============================================================================= 1 failed, 1 passed in 0.06s ============================================================================= ```
* Adding test_unique_ptr_member (for desired PyCLIF behavior). See also: #2583 Does not build with upstream master or #2047, but builds with https://github.com/RobotLocomotion/pybind11 and almost runs: ``` Running tests in directory "/usr/local/google/home/rwgk/forked/EricCousineau-TRI/pybind11/tests": ================================================================================= test session starts ================================================================================= platform linux -- Python 3.8.5, pytest-5.4.3, py-1.9.0, pluggy-0.13.1 rootdir: /usr/local/google/home/rwgk/forked/EricCousineau-TRI/pybind11/tests, inifile: pytest.ini collected 2 items test_unique_ptr_member.py .F [100%] ====================================================================================== FAILURES ======================================================================================= _____________________________________________________________________________ test_pointee_and_ptr_owner ______________________________________________________________________________ def test_pointee_and_ptr_owner(): obj = m.pointee() assert obj.get_int() == 213 m.ptr_owner(obj) with pytest.raises(ValueError) as exc_info: > obj.get_int() E Failed: DID NOT RAISE <class 'ValueError'> test_unique_ptr_member.py:17: Failed ============================================================================= 1 failed, 1 passed in 0.06s ============================================================================= ``` * unique_ptr or shared_ptr return * new test_variant_unique_shared with vptr_holder prototype * moving prototype code to pybind11/vptr_holder.h, adding type_caster specialization to make the bindings involving unique_ptr passing compile, but load and cast implementations are missing * disabling GitHub Actions on pull_request (for this PR) * disabling AppVeyor (for this PR) * TRIGGER_SEGSEV macro, annotations for GET_STACK (vptr::get), GET_INT_STACK (pointee) * adding test_promotion_of_disowned_to_shared * Copying tests as-is from xxx_value_ptr_xxx_holder branch. https://github.com/rwgk/pybind11/tree/xxx_value_ptr_xxx_holder Systematically exercising returning and passing unique_ptr<T>, shared_ptr<T> with unique_ptr, shared_ptr holder. Observations: test_holder_unique_ptr: make_unique_pointee OK pass_unique_pointee BUILD_FAIL (as documented) make_shared_pointee Abort free(): double free detected pass_shared_pointee RuntimeError: Unable to load a custom holder type from a default-holder instance test_holder_shared_ptr: make_unique_pointee Segmentation fault (#1138) pass_unique_pointee BUILD_FAIL (as documented) make_shared_pointee OK pass_shared_pointee OK * Copying tests as-is from xxx_value_ptr_xxx_holder branch. https://github.com/rwgk/pybind11/tree/xxx_value_ptr_xxx_holder Systematically exercising casting between shared_ptr<base>, shared_ptr<derived>. * Demonstration of Undefined Behavior in handling of shared_ptr holder. Based on https://godbolt.org/z/4fdjaW by jorgbrown@ (thanks Jorg!). * Additional demonstration of Undefined Behavior in handling of shared_ptr holder. * fixing up-down mixup in comment * Demonstration of Undefined Behavior in handling of polymorphic pointers. (This demo does NOT involve smart pointers at all, unlike the otherwise similar test_smart_ptr_private_first_base.) * minor test_private_first_base.cpp simplification (after discovering that this can be wrapped with Boost.Python, using boost::noncopyable) * pybind11 equivalent of Boost.Python test similar to reproducer under #1333 * Snapshot of WIP, TODO: shared_ptr deleter with on/off switch * Adding vptr_deleter. * Adding from/as unique_ptr<T> and unique_ptr<T, D>. * Adding from_shared_ptr. Some polishing. * New tests/core/smart_holder_poc_test.cpp, using Catch2. * Adding in vptr_deleter_guard_flag. * Improved labeling of TEST_CASEs. * Shuffling existing TEST_CASEs into systematic matrix. * Implementing all [S]uccess tests. * Implementing all [E]xception tests. * Testing of exceptions not covered by the from-as matrix. * Adding top-level comment. * Converting from methods to factory functions (no functional change). * Removing obsolete and very incomplete test (replaced by Catch2-based test). * Removing stray file. * Adding type_caster_bare_interface_demo. * Adding shared_ptr<mpty>, shared_ptr<mpty const> casters. * Adding unique_ptr<mpty>, unique_ptr<mpty const> casters. * Pure copy of `class class_` implementation in pybind11.h (master commit 98f1bbb). * classh.h: renaming of class_ to classh + namespace; forking test_classh_wip from test_type_caster_bare_interface_demo. * Hard-coding smart_holder into classh. * Adding mpty::mtxt string member. * Adding isinstance<mpty> in type_caster::load functions. * Adding rvalue_ref, renaming const_value_ref to lvalue_ref & removing const. * Retrieving smart_holder pointer in type_caster<mpty>::load, and using it cast_op operators. * Factoring out smart_holder_type_caster_load. * Retrieving smart_holder pointer in type_caster<std::shared_ptr<mpty[ const]>>::load, and using it cast_op operators. * Improved error messaging: Cannot disown nullptr (as_unique_ptr). * Retrieving smart_holder pointer in type_caster<std::unique_ptr<mpty[ const]>>::load, and using it cast_op operators. * Pure `clang-format --style=file -i` change. * Pure `clang-format --style=file -i` change, with two `clang-format off` directives. * Fixing oversight (discovered by flake8). * flake8 cleanup * Systematically setting mtxt for all rtrn_mpty_* functions (preparation, the values are not actually used yet). * static cast handle for rtrn_cptr works by simply dropping in code from type_caster_base (marked with comments). * static cast handle for rtrn_cref works by simply dropping in code from type_caster_base (marked with comments). rtrn_mref and rtrn_mptr work via const_cast (to add const). * static cast handle for rtrn_valu works by simply dropping in code from type_caster_base (marked with comments). rtrn_rref raises a RuntimeError, to be investigated. * Copying type_caster_generic::cast into type_caster<mpty> as-is (preparation for handling smart pointers). * Pure clang-format change (applied to original type_caster_generic::cast). * Adding comment re potential use_count data race. * static handle cast implementations for rtrn_shmp, rtrn_shcp. * Adding MISSING comments in operator std::unique_ptr<mpty[ const]>. * static handle cast implementations for rtrn_uqmp, rtrn_uqcp. * Bug fix: vptr_deleter_armed_flag_ptr has to live on the heap. See new bullet point in comment section near the top. The variable was also renamed to reflect its function more accurately. * Fixing bugs discovered by ASAN. The code is now ASAN, MSAN, UBSAN clean. * Making test_type_caster_bare_interface_demo.cpp slightly more realistic, ASAN, MSAN, UBSAN clean. * Calling deregister_instance after disowning via unique_ptr. * Removing enable_shared_from_this stub, simplifying existing code, clang-format. Open question, with respect to the original code: https://github.com/pybind/pybind11/blob/76a160070b369f8d82b945c97924227e8b835c94/include/pybind11/pybind11.h#L1510 To me it looks like the exact situation marked as `std::shared_ptr<Good> gp1 = not_so_good.getptr();` here: https://en.cppreference.com/w/cpp/memory/enable_shared_from_this The comment there is: `// undefined behavior (until C++17) and std::bad_weak_ptr thrown (since C++17)` Does the existing code have UB pre C++17? I'll leave handling of enable_shared_from_this for later, as the need arises. * Cosmetical change around helper functions. * Using type_caster_base<mpty>::src_and_type directly, removing copy. Also renaming one cast to cast_const_raw_ptr, for clarity. * Fixing clang-format oversight. * Using factored-out make_constructor (PR #2798), removing duplicate code. * Inserting additional assert to ensure a returned unique_ptr is always a new Python instance. * Adding minor comment (change to internals needed to distinguish uninitialized/disowned in error message). * Factoring out find_existing_python_instance(). * Moving factored-out make_constructor to test_classh_wip.cpp, restoring previous version of cast.h. This is currently the most practical approach. See PR #2798 for background. * Copying classh type_casters from test_classh_wip.cpp UNMODIFIED, as a baseline for generalizing the code. * Using pybind11/detail/classh_type_casters.h from test_classh_wip.cpp. * Adding & using PYBIND11_CLASSH_TYPE_CASTERS define. * Adding test_classh_inheritance, currently failing (passes with class_). * Removing .clang-format before git rebase master (where the file was added). * Bringing back .clang-format, the previous rm was a bad idea. * Folding in modified_type_caster_generic_load_impl, just enough to pass test_class_wip. test_classh_inheritance is still failing, but with a different error: [RuntimeError: Incompatible type (as_raw_ptr_unowned).] * Minimal changes needed to pass test_classh_inheritance. * First pass adjusting try_implicit_casts and try_load_foreign_module_local to capture loaded_v_h, but untested and guarded with pybind11_failure("Untested"). This was done mainly to determine general feasibility. Note the TODO in pybind11.h, where type_caster_generic::local_load is currently hard-coded. test_classh_wip and test_classh_inheritance still pass, as before. * Decoupling generic_type from type_caster_generic. * Changes and tests covering classh_type_casters try_implicit_casts. * Minimal test covering classh_type_casters load_impl Case 2b. * Removing stray isinstance<T>(src): it interferes with the py::module_local feature. Adding missing #includes. * Tests for classh py::module_local() feature. * Pure renaming of function names in test_classh_inheritance, similar to the systematic approach used in test_class_wip. NO functional changes. * Pure renaming of function and variable names, for better generalization when convoluting with inheritance. NO functional changes. * Adopting systematic naming scheme from test_classh_wip. NO functional changes. * Moving const after type name, for functions that cover a systematic scheme. NO functional changes. * Adding smart_holder_type_caster_load::loaded_as_shared_ptr, currently bypassing smart_holder shared_ptr tracking completely, but the tests pass and are sanitizer clean. * Removing rtti_held from smart_holder. See updated comment. * Cleaning up loaded_as_raw_ptr_unowned, loaded_as_shared_ptr. * Factoring out convert_type and folding into loaded_as_unique_ptr. * Folding convert_type into lvalue_ref and rvalue_ref paths. Some smart_holder_type_caster_load cleanup. * Using unique_ptr in local_load to replace static variable. Also adding local_load_safety_guard. * Converting test_unique_ptr_member to using classh: fully working, ASAN, MSAN, UBSAN clean. * Removing debugging comments (GET_STACK, GET_INT_STACK). cast.h is identical to current master again, pybind11.h only has the generic_type::initialize(..., &type_caster_generic::local_load) change. * Purging obsolete pybind11/vptr_holder.h and associated test. * Moving several tests to github.com/rwgk/rwgk_tbx/tree/main/pybind11_tests rwgk/rwgk_tbx@a2c2f88 These tests are from experimenting, and for demonstrating UB in pybind11 multiple inheritance handling ("first_base"), to be fixed later. * Adding py::smart_holder support to py::class_, purging py::classh completely. * Renaming files in include directory, creating pybind11/smart_holder.h. * Renaming all "classh" to "smart_holder" in pybind11/detail/smart_holder_type_casters.h. The user-facing macro is now PYBIND11_SMART_HOLDER_TYPE_CASTERS. * Systematically renaming tests to use "class_sh" in the name. * Renaming test_type_caster_bare_interface_demo to test_type_caster_bare_interface. * Renaming new tests/core subdirectory to tests/pure_cpp. * Adding new tests to CMake config, resetting CI config. * Changing CMake file so that test_class_sh_module_local.py actually runs. * clang-tidy fixes. * 32-bit compatibility. * Reusing type_caster_base make_copy_constructor, make_move_constructor with a trick. * CMake COMPARE NATURAL is not available with older versions. * Adding copyright notices to new header files. * Explicitly define copy/move constructors/assignments. * Adding new header files to tests/extra_python_package/test_files.py. * Adding tests/pure_cpp/CMakeLists.txt. * Making use of the new find_existing_python_instance() function factored out with PR #2822. * Moving define PYBIND11_SMART_HOLDER_TYPE_CASTERS(T) down in the file. NO functional changes. Preparation for follow-up work (to keep that diff smaller). * Reintroducing py::classh, this time as a simple alias for py::class_<U, py::smart_holder>. * Replacing detail::is_smart_holder<H> in cast.h with detail::is_smart_holder_type_caster<T>. Moving get_local_load_function_ptr, init_instance_for_type to smart_holder_type_caster_class_hooks. Expanding static_assert in py::type::handle_of<> to accommodate smart_holder_type_casters. * Fixing oversight. * Adding classu alias for class_<U, std::unique_ptr<U>>. * Giving up on idea to use legacy init_instance only if is_base_of<type_caster_generic, type_caster<T>. There are use cases in the wild that define both a custom type_caster and class_. * Removing test_type_caster_bare_interface, which was moved to the separate PR #2834. * Moving up is_smart_holder_type_caster, to also use in cast_is_temporary_value_reference. * Adding smart_holder_type_casters for unique_ptr with custom deleter. SEVERE CODE DUPLICATION. This commit is to establish a baseline for consolidating the unique_ptr code. * Unification of unique_ptr, unique_ptr_with_deleter code in smart_holder_poc.h. Leads to more fitting error messages. Enables use of unique_ptr<T, D> smart_holder_type_casters also for unique_ptr<T>. * Copying files as-is from branch test_unique_ptr_member (PR #2672). * Adding comment, simplifying naming, cmake addition. * Introducing PYBIND11_USE_SMART_HOLDER_AS_DEFAULT macro (tested only undefined; there are many errors with the macro defined). * Removing test_type_caster_bare_interface, which was moved to the separate PR #2834. * Fixing oversight introduced with commit 95425f1. * Setting record.default_holder correctly for PYBIND11_USE_SMART_HOLDER_AS_DEFAULT. With this test_class.cpp builds and even mostly runs, except `test_multiple_instances_with_same_pointer`, which segfaults because it is using a `unique_ptr` holder but `smart_holder` `type_caster`. Also adding `static_assert`s to generate build errors for such situations, but guarding with `#if 0` to first pivot to test_factory_constructors.cpp. * Fixing up cast.h and smart_holder.h after rebase. * Removing detail/smart_holder_type_casters.h in separate commit. * Commenting out const in def_buffer(... const). With this, test_buffers builds and runs with PYBIND11_USE_SMART_HOLDER_AS_DEFAULT. Explanation why the const needs to be removed, or fix elsewhere, is still needed, but left for later. * Adding test_class_sh_factory_constructors, reproducing test_factory_constructors failure. Using py::class_ in this commit, to be changed to py::classh for debugging. * Removing include/pybind11/detail/smart_holder_type_casters.h from CMakeLists.txt, test_files.py (since it does not exist in this branch). * Adding // DANGER ZONE reminders. * Converting as many py::class_ to py::classh as possible, not breaking tests. * Adding initimpl::construct() overloads, resulting in test_class_sh_factory_constructors feature parity for py::class_ and py::classh. * Adding enable_if !is_smart_holder_type_caster to existing initimpl::construct(). With this test_factory_constructors.cpp builds with PYBIND11_USE_SMART_HOLDER_AS_DEFAULT. * Disabling shared_ptr&, shared_ptr* tests when building with PYBIND11_USE_SMART_HOLDER_AS_DEFAULT for now, pending work on smart_holder_type_caster<shared_ptr>. * Factoring out struct and class definitions into anonymous namespace. Preparation for building with PYBIND11_USE_SMART_HOLDER_AS_DEFAULT. * Simplifying from_unique_ptr(): typename D = std::default_delete<T> is not needed. Factoring out is_std_default_delete<T>() for consistentcy between ensure_compatible_rtti_uqp_del() and from_unique_ptr(). * Introducing PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS. Using it in test_smart_ptr.cpp. With this test_smart_ptr builds with PYBIND11_USE_SMART_HOLDER_AS_DEFAULT and all but one test run successfully. * Introducing 1. type_caster_for_class_, used in PYBIND11_MAKE_OPAQUE, and 2. default_holder_type, used in stl_bind.h. * Using __VA_ARGS__ in PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS. * Replacing condense_for_macro with much simpler approach. * Softening static_assert, to only check specifically that smart_holder is not mixed with type_caster_base, and unique_ptr/shared_ptr holders are not mixed with smart_holder_type_casters. * Adding PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS in test_class.cpp (with this all but one test succeed with PYBIND11_USE_SMART_HOLDER_AS_DEFAULT). * Adding remaining PYBIND11_SMART_POINTER_HOLDER_TYPE_CASTERS. static_assert for "necessary conditions" for both types of default holder, static_assert for "strict conditions" guarded by new PYBIND11_STRICT_ASSERTS_CLASS_HOLDER_VS_TYPE_CASTER_MIX. All tests build & run as before with unique_ptr as the default holder, all tests build for smart_holder as the default holder, even with the strict static_assert. * Introducing check_is_smart_holder_type_caster() function for runtime check, and reinterpreting record.default_holder as "uses_unique_ptr_holder". With this test_smart_ptr succeeds. (All 42 tests build, 35 tests succeed, 5 run but have some failures, 2 segfault.) * Bug fix: Adding have_value() to smart_holder_type_caster_load. With this test_builtin_casters succeeds. (All 42 tests build, 36 tests succeed, 5 run but have some failures, 1 segfault.) * Adding unowned_void_ptr_from_direct_conversion to modified_type_caster_generic_load_impl. This fixes the last remaining segfault (test_numpy_dtypes). New stats for all tests combined: 12 failed, 458 passed. * Adding "Lazy allocation for unallocated values" (for old-style __init__) into load_value_and_holder. Deferring destruction of disowned holder until clear_instance, to remain inspectable for "uninitialized" or "disowned" detection. New stats for all tests combined: 5 failed, 465 passed. * Changing std::shared_ptr pointer/reference to const pointer/reference. New stats for all tests combined: 4 failed, 466 passed. * Adding return_value_policy::move to permissible policies for unique_ptr returns. New stats for all tests combined: 3 failed, 467 passed. * Overlooked flake8 fixes. * Manipulating failing ConstructorStats test to pass, to be able to run all tests with ASAN. This version of the code is ASAN clean with unique_ptr or smart_holder as the default. This change needs to be reverted after adopting the existing move-only-if-refcount-is-1 logic used by type_caster_base. * Adding copy constructor and move constructor tracking to atyp. Preparation for a follow-up change in smart_holder_type_caster, to make this test sensitive to the changing behavior. [skip ci] * Removing `operator T&&() &&` from smart_holder_type_caster, for compatibility with the behavior of type_caster_base. Enables reverting 2 of 3 test manipulations applied under commit 249df7c. The manipulation in test_factory_constructors.py is NOT reverted in this commit. [skip ci] * Fixing unfortunate editing mishap. This reverts the last remaining test manipulation in commit 249df7c and makes all existing unit tests pass with smart_holder as default holder. * GitHub CI clang-tidy fixes. * Adding messages to terse `static_assert`s, for pre-C++17 compatibility. * Using @pytest.mark.parametrize to run each assert separately (to see all errors, not just the first). * Systematically removing _atyp from function names, to make the test code simpler. * Using re.match to accommodate variable number of intermediate MvCtor. * Also removing `operator T()` from smart_holder_type_caster, to fix gcc compilation errors. The only loss is pass_rref in test_class_sh_basic. * Systematically replacing `detail::enable_if_t<...smart_holder...>` with `typename std::enable_if<...smart_holder...>::type`. Attempt to work around MSVC 2015 issues, to be tested via GitHub CI. The idea for this change originates from this comment: #1616 (comment) * Importing re before pytest after observing a PyPy CI flake when importing pytest first. * Copying MSVC 2015 compatibility change from branch pr2672_use_smart_holder_as_default. * Introducing is_smart_holder_type_caster_base_tag, to keep smart_holder code more disconnected. * Working around MSVC 2015 bug. * Expanding comment for MSVC 2015 workaround. * Systematically changing std::enable_if back to detail::enable_if_t, effectively reverting commit 5d4b689. * Removing unused smart_holder_type_caster_load::loaded_as_rvalue_ref (it was an oversight that it was not removed with commit 23036a4). * Removing py::classu, because it does not seem useful enough. * Reverting commit 6349531 by un-commenting `const` in `def_buffer(...)`. To make this possible, `operator T const&` and `operator T const*` in `smart_holder_type_caster` need to be marked as `const` member functions. * Adding construct() overloads for constructing smart_holder from alias unique_ptr, shared_ptr returns. * Adding test_class_sh_factory_constructors.cpp to tests/CMakeLists.txt (fixes oversight, this should have been added long before). * Compatibility with old clang versions (clang 3.6, 3.7 C++11). * Cleaning up changes to existing unit tests. * Systematically adding SMART_HOLDER_WIP tag. Removing minor UNTESTED tags (only the throw are not actually exercised, investing time there has a high cost but very little benefit). * Splitting out smart_holder_type_casters again, into new detail/smart_holder_type_casters_inline_include.h. * Splitting out smart_holder_init_inline_include.h. * Adding additional new include files to CMakeLists.txt, tests/extra_python_package/test_files.py. * clang-format cleanup of most smart_holder code. * Adding source code comments in response to review. * Simple micro-benchmark ("ubench") comparing runtime performance for several holders. Tested using github.com/rwgk/pybind11_scons and Google-internal build system. Sorry, no cmake support at the moment. First results: https://docs.google.com/spreadsheets/d/1InapCYws2Gt-stmFf_Bwl33eOMo3aLE_gc9adveY7RU/edit#gid=0 * Breaking out number_bucket.h, adding hook for also collecting performance data for PyCLIF. * Accounting for ubench in MANIFEST.in (simply prune, for now). * Smarter determination of call_repetitions. [skip ci] * Also scaling performance data to PyCLIF. [skip ci] * Adding ubench/python/number_bucket.clif here for general visibility. * Fix after rebase * Merging detail/smart_holder_init_inline_include.h into detail/init.h. * Renaming detail/is_smart_holder_type_caster.h -> detail/smart_holder_sfinae_hooks_only.h. * Renaming is_smart_holder_type_caster -> type_uses_smart_holder_type_caster for clarity. * Renaming type_caster_type_is_smart_holder_type_caster -> wrapped_type_uses_smart_holder_type_caster for clarity. * Renaming is_smart_holder_type_caster_base_tag -> smart_holder_type_caster_base_tag for simplicity. * Adding copyright notices and minor colateral cleanup. * iwyu cleanup (comprehensive only for cast.h and smart_holder*.h files). * Fixing `git rebase master` accident. * Moving large `pragma warning` block from pybind11.h to detail/common.h. * Fixing another `git rebase master` accident.
Don't enforce moving by passing the caster as an rvalue to cast_op().
… function signature
So far, using the movable_cast_op_type, always enabled move semantics,
even when copy semantics would be expected from the function signature.
While C++ decides about copy vs move semantics based on the type of the passed argument,
Python doesn't know about lvalue and rvalue references and thus cannot decide based on the
passed argument. Instead, the wrapper needs to explicitly define the desired semantics of
argument passing as follows:
- f(T&) passes an lvalue reference
- f(T&&) passes an rvalue reference
- f(T) passes an lvalue reference by default (triggering copy construction)
but falls back to an rvalue reference if copying is not supported (thus triggering move construction)
This commit essentially makes call_impl() pass the argument casters to cast_op() by lvalue references, thus
enabling the distinction between copy and move semantics based on the argument's type.
As we prefer copying over moving now (if not explicitly asked for otherwise), the constructor stats shift from moving towards copying.
rhaschke
force-pushed
the
rvalue-argument-fix1
branch
from
09d487d to
aa7d99e
Compare
|
I rebased onto master and reworked the PR thoroughly - once again. Particularly, I gave up the idea to automatically trigger a move construction when passing into an rvalue argument. Now, both lvalue and rvalue arguments don't trigger any constructor, they just pass the reference (as in C++). However, in contrast to the previous Actually, I suggest to use this new |
|
I'm not sure why the clang 3.6/3.7 builds fail with an error referring to gcc includes. |
Yes, that's a known flake, I saw it the first time ~2-3 weeks ago. Seems to be intermittently failing download attempts. If you try again it'll probably work. |
I'm pretty sure that gcc error is very easy to work around, I ran into it several times, too. You may only need some of the 4 lines there, but I just put in all, to not have to go through a time-consuming back-and-forth with the CI. |
I think ideally one
It would be nice to develop a new "Understanding copy and move semantics for wrapped function arguments" section along with this PR, that explains the new behavior in a systematic way (for people new to pybind11), and contrasts it to the old behavior (for people who have existing wrappers), with tips what to watch out for when upgrading. |
clang 3.x requires declaration of default copy constructors, due to use of std::is_copy_constructible.
rhaschke
force-pushed
the
rvalue-argument-fix1
branch
from
159725d to
b000987
Compare
Not exactly. If the wrapped function arg is an lvalue (&) or rvalue (&&) reference, the argument is just passed as a reference. Thus, neither copying nor moving is performed (yet). Copying or moving only occurs if the argument is declared by-value.
No, unfortunately not. I thought about using the refcount to automatically decide between lvalue and rvalue references. That would be actually the most elegant solution, because unique python objects (with refcount = 1) could be automatically passed as rvalue references. However, this still wouldn't allow to explicitly move an argument.
I think trying to do so, will result in a compile error, because the lambda function cannot pass an lvalue reference to the rvalue reference of the underlying function. There you need to use
I thought we might need a |
|
Looks like I'm still miles off with my understanding. Sorry. I did experiment myself a little bit while working on the smart_holder_type_caster, but apparently I misunderstood a lot what actually happens. FWIW, I started out with this (which I got mostly from PR #2705): I got pretty far with this, but eventually I wound up removing the first two operators, simply driven by wanting to be compatible with
I'm not sure this is helping here directly, but I think it would be great to have something similar to |
Vice versa, I couldn't really understand the aim of your referenced PRs. Shall we have a short video chat to clarify?
I agree and I think, that's what was done in tests/test_copy_move.py : |
|
Being able to use forwarding references of custom classes would be a game changer :) |
|
Off the top of my head: I think we ran into issues with trampolines that are very tricky to resolve. Possibly, if the desired feature had been taken into account early on, it may have been easier to accommodate it, but now it's "too late". Unless someone high-powered & with a good block of time jumps on this, I don't think this will get done anytime soon. My own assessment: a nice to have. Certainly not a game changer. |
|
This feature is provided by the |
The latest smart_holder branch (as of #5257) is using pybind11/detail/smart_holder_type_casters.h is gone completely. Is it still true now that "this feature is provided by the |
Maybe you are right. The key feature of the smart_holder branch I'm using are rvalue holder arguments. |
pybind11 doesn't support rvalue function arguments for custom types, i.e. the following function cannot be wrapped:
This issue is (more or less) unrelated to rvalue holder arguments as requested e.g. in #2040 or #2583.
While moving holder arguments essentially transfers ownership from python to C++, which is admittedly difficult, supporting functions like above shouldn't pose a conceptual problem:
The original object instance remains existing in python, but only its internals are moved over to C++.
Currently, this use case is not supported, because the
cast_op_typeprovided bytype_caster_base<T>doesn't support moving. Replacing it withmovable_cast_op_type<T>and providing a corresponding cast operator already solves the problem for my example.However, this solution makes moving preferred over copying, which isn't desired either.
EDIT: A major rework of this PR solves this issue, see #2047 (comment) and #2047 (comment) for more details.