From fe755dce12766820a99eefbde32d6ceb0a828ca8 Mon Sep 17 00:00:00 2001 From: Dustin Spicuzza Date: Sun, 24 Nov 2019 02:36:48 -0500 Subject: [PATCH 1/9] Add AutoWIG to list of binding generators (#1990) * Add AutoWIG to list of binding generators --- docs/compiling.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/compiling.rst b/docs/compiling.rst index c50c7d8afb..496410ca25 100644 --- a/docs/compiling.rst +++ b/docs/compiling.rst @@ -287,3 +287,11 @@ code by introspecting existing C++ codebases using LLVM/Clang. See the [binder]_ documentation for details. .. [binder] http://cppbinder.readthedocs.io/en/latest/about.html + +[AutoWIG]_ is a Python library that wraps automatically compiled libraries into +high-level languages. It parses C++ code using LLVM/Clang technologies and +generates the wrappers using the Mako templating engine. The approach is automatic, +extensible, and applies to very complex C++ libraries, composed of thousands of +classes or incorporating modern meta-programming constructs. + +.. [AutoWIG] https://github.com/StatisKit/AutoWIG From 98ea003e66c30d528ebacf48c0f3ca474c56f74f Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Fri, 20 Dec 2019 13:30:55 +0100 Subject: [PATCH 2/9] failing test --- tests/CMakeLists.txt | 2 +- tests/test_move_arg.cpp | 31 +++++++++++++++++++++++++++++++ tests/test_move_arg.h | 34 ++++++++++++++++++++++++++++++++++ tests/test_move_arg.py | 14 ++++++++++++++ 4 files changed, 80 insertions(+), 1 deletion(-) create mode 100644 tests/test_move_arg.cpp create mode 100644 tests/test_move_arg.h create mode 100644 tests/test_move_arg.py diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 765c47adb0..8208290c91 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -151,7 +151,7 @@ function(pybind11_enable_warnings target_name) endif() endfunction() -set(test_targets pybind11_tests) +set(test_targets pybind11_tests test_move_arg) # Build pybind11_cross_module_tests if any test_whatever.py are being built that require it foreach(t ${PYBIND11_CROSS_MODULE_TESTS}) diff --git a/tests/test_move_arg.cpp b/tests/test_move_arg.cpp new file mode 100644 index 0000000000..9cc1a75e56 --- /dev/null +++ b/tests/test_move_arg.cpp @@ -0,0 +1,31 @@ +#include "test_move_arg.h" +#include +#include +#include + +namespace py = pybind11; + +PYBIND11_MODULE(test_move_arg, m) { + py::class_(m, "Item") + .def(py::init(), py::call_guard()) + .def("__repr__", [](const Item& item) { + std::stringstream ss; + ss << "py " << item; + return ss.str(); + }, py::call_guard()); + + m.def("access", [](const Item& item) { + std::cout << "access " << item << "\n"; + }, py::call_guard()); + +#if 0 // rvalue arguments fail during compilation + m.def("consume", [](Item&& item) { + std::cout << "consume " << item << "\n "; + Item sink(std::move(item)); + std::cout << " old: " << item << "\n new: " << sink << "\n"; + }, py::call_guard()); +#endif + + m.def("working", [](int&& a) { std::cout << a << "\n"; }, + py::call_guard()); +} diff --git a/tests/test_move_arg.h b/tests/test_move_arg.h new file mode 100644 index 0000000000..afd9228379 --- /dev/null +++ b/tests/test_move_arg.h @@ -0,0 +1,34 @@ +#pragma once +#include +#include +#include + +class Item; +std::ostream& operator<<(std::ostream& os, const Item& item); + +/** Item class requires unique instances, i.e. only supports move construction */ +class Item +{ +public: + Item(int value) : value_(std::make_unique(value)) { + std::cout<< "new " << *this << "\n"; + } + ~Item() { + std::cout << "destroy " << *this << "\n"; + } + Item(const Item&) = delete; + Item(Item&& other) { + std::cout << "move " << other << " -> "; + value_ = std::move(other.value_); + std::cout << *this << "\n"; + } + + std::unique_ptr value_; +}; + +std::ostream& operator<<(std::ostream& os, const Item& item) { + os << "item " << &item << "("; + if (item.value_) os << *item.value_; + os << ")"; + return os; +} diff --git a/tests/test_move_arg.py b/tests/test_move_arg.py new file mode 100644 index 0000000000..d95a2ccdbb --- /dev/null +++ b/tests/test_move_arg.py @@ -0,0 +1,14 @@ +import pytest +from test_move_arg import Item, access + + +def test(): + item = Item(42) + other = item + access(item) + print(item) + del item + print(other) + +if __name__ == "__main__": + test() From 15629209328af1b45efd4399211e95f95e02006e Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Sun, 29 Dec 2019 11:34:48 +0100 Subject: [PATCH 3/9] consume(std::unique_ptr&&) --- tests/test_move_arg.cpp | 9 +++++++-- tests/test_move_arg.py | 13 +++++++++++-- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/tests/test_move_arg.cpp b/tests/test_move_arg.cpp index 9cc1a75e56..487e945f7f 100644 --- a/tests/test_move_arg.cpp +++ b/tests/test_move_arg.cpp @@ -26,6 +26,11 @@ PYBIND11_MODULE(test_move_arg, m) { }, py::call_guard()); #endif - m.def("working", [](int&& a) { std::cout << a << "\n"; }, - py::call_guard()); + m.def("consume", [](std::unique_ptr&& item) { + std::cout << "consume " << *item.get() << "\n"; + std::unique_ptr sink(std::move(item)); + std::cout << " old: " << item.get() << "\n new: " << *sink.get() << "\n"; + }, py::call_guard()); + + m.def("consume_str", [](std::string&& s) { std::string o(std::move(s)); }); } diff --git a/tests/test_move_arg.py b/tests/test_move_arg.py index d95a2ccdbb..ed319e17b7 100644 --- a/tests/test_move_arg.py +++ b/tests/test_move_arg.py @@ -1,5 +1,5 @@ import pytest -from test_move_arg import Item, access +from test_move_arg import * def test(): @@ -10,5 +10,14 @@ def test(): del item print(other) +def test_produce(): + item = Item(42) + access(item) + consume(item) + access(item) + +def test_foo(): + foo() + if __name__ == "__main__": - test() + test_produce() From 16bb1838ac59066a1e674844ed5e81c9ccc099a6 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Mon, 30 Dec 2019 11:55:23 +0100 Subject: [PATCH 4/9] new move_only_holder_caster --- include/pybind11/cast.h | 83 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 77 insertions(+), 6 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 605acb3668..3decc49b8c 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1515,15 +1515,86 @@ template class type_caster> : public copyable_holder_caster> { }; template -struct move_only_holder_caster { - static_assert(std::is_base_of, type_caster>::value, +struct move_only_holder_caster : public type_caster_base { +public: + using base = type_caster_base; + static_assert(std::is_base_of>::value, "Holder classes are only supported for custom types"); + using base::base; + using base::cast; + using base::typeinfo; + using base::value; + + bool load(handle& src, bool convert) { + bool success = base::template load_impl>(src, convert); + if (success) // if loading was successful, the instance needs to be withdrawn from src + // However, setting the src pointer to None is not sufficient. + src.ptr() = none().release().ptr(); + return success; + } + + template using cast_op_type = detail::movable_cast_op_type; + + explicit operator type*() { return this->value; } + explicit operator type&() { return *(this->value); } + explicit operator holder_type*() { return std::addressof(holder); } + + // Workaround for Intel compiler bug + // see pybind11 issue 94 + #if defined(__ICC) || defined(__INTEL_COMPILER) + operator holder_type&() { return holder; } + #else + explicit operator holder_type&() { return holder; } + #endif + explicit operator holder_type&&() { value = nullptr; return std::move(holder); } + + static handle cast(const holder_type &src, return_value_policy, handle) { + const auto *ptr = holder_helper::get(src); + return type_caster_base::cast_holder(ptr, &src); + } + +protected: + friend class type_caster_generic; + void check_holder_compat() { +// if (typeinfo->default_holder) +// throw cast_error("Unable to load a custom holder type from a default-holder instance"); + } - static handle cast(holder_type &&src, return_value_policy, handle) { - auto *ptr = holder_helper::get(src); - return type_caster_base::cast_holder(ptr, std::addressof(src)); + bool load_value(value_and_holder &&v_h) { + if (v_h.holder_constructed()) { + value = v_h.value_ptr(); + holder = std::move(v_h.template holder()); + return true; + } else { + throw cast_error("Unable to cast from non-held to held instance (T& to Holder) " +#if defined(NDEBUG) + "(compile in debug mode for type information)"); +#else + "of type '" + type_id() + "''"); +#endif + } + } + + template ::value, int> = 0> + bool try_implicit_casts(handle, bool) { return false; } + + template ::value, int> = 0> + bool try_implicit_casts(handle src, bool convert) { + for (auto &cast : typeinfo->implicit_casts) { + move_only_holder_caster sub_caster(*cast.first); + if (sub_caster.load(src, convert)) { + value = cast.second(sub_caster.value); + holder = holder_type(sub_caster.holder, (type *) value); + return true; + } + } + return false; } - static constexpr auto name = type_caster_base::name; + + static bool try_direct_conversions(handle) { return false; } + + + holder_type holder; }; template From b78e8e315f0feca0924b300c9a60e1a168f9e3ad Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Mon, 30 Dec 2019 14:18:10 +0100 Subject: [PATCH 5/9] argument_loader doesn't consider actual argument types --- include/pybind11/cast.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 3decc49b8c..6e58d8db78 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1995,6 +1995,8 @@ class argument_loader { template bool load_impl_sequence(function_call &call, index_sequence) { + // BUG: When loading the arguments, the actual argument type (pointer, lvalue reference, rvalue reference) + // is already lost (argcasters only know the intrinsic type), while the behaviour should differ for them, e.g. for rvalue references. for (bool r : {std::get(argcasters).load(call.args[Is], call.args_convert[Is])...}) if (!r) return false; From 8d1b2e8412b0b4e7de980259a537e53e6afe0803 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Mon, 30 Dec 2019 19:33:42 +0100 Subject: [PATCH 6/9] custom holder type --- tests/test_move_arg.cpp | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/tests/test_move_arg.cpp b/tests/test_move_arg.cpp index 487e945f7f..1f60d4bff8 100644 --- a/tests/test_move_arg.cpp +++ b/tests/test_move_arg.cpp @@ -5,8 +5,35 @@ namespace py = pybind11; +#if 1 +template +class my_ptr { +public: + my_ptr(T* p = nullptr) : ptr_(p) {} + my_ptr(my_ptr&& other) : ptr_(other.ptr_) { other.ptr_ = nullptr; } + ~my_ptr() { delete ptr_; } + my_ptr& operator=(my_ptr&& other) { ptr_ = other.ptr_; other.ptr_ = nullptr; return *this; } + const T* get() const { return ptr_; } + const T* verbose_get() const { + std::cout << " [" << ptr_ << "] "; return ptr_; + } +private: + T* ptr_; +}; +PYBIND11_DECLARE_HOLDER_TYPE(T, my_ptr) +namespace pybind11 { namespace detail { + template + struct holder_helper> { // <-- specialization + static const T *get(const my_ptr &p) { return p.verbose_get(); } + }; +}} +#else +template +using my_ptr = std::unique_ptr; +#endif + PYBIND11_MODULE(test_move_arg, m) { - py::class_(m, "Item") + py::class_>(m, "Item") .def(py::init(), py::call_guard()) .def("__repr__", [](const Item& item) { std::stringstream ss; @@ -26,9 +53,9 @@ PYBIND11_MODULE(test_move_arg, m) { }, py::call_guard()); #endif - m.def("consume", [](std::unique_ptr&& item) { + m.def("consume", [](my_ptr&& item) { std::cout << "consume " << *item.get() << "\n"; - std::unique_ptr sink(std::move(item)); + my_ptr sink(std::move(item)); std::cout << " old: " << item.get() << "\n new: " << *sink.get() << "\n"; }, py::call_guard()); From e8b029c816098d3aab75501e5527dfb3dad734f3 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Mon, 30 Dec 2019 20:53:55 +0100 Subject: [PATCH 7/9] fixup! new move_only_holder_caster --- include/pybind11/cast.h | 38 +++++++++++++++----------------------- 1 file changed, 15 insertions(+), 23 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 6e58d8db78..90b6104138 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1527,9 +1527,8 @@ struct move_only_holder_caster : public type_caster_base { bool load(handle& src, bool convert) { bool success = base::template load_impl>(src, convert); - if (success) // if loading was successful, the instance needs to be withdrawn from src - // However, setting the src pointer to None is not sufficient. - src.ptr() = none().release().ptr(); + if (success) // On success, remember src pointer to withdraw later + src_handle = std::addressof(src); return success; } @@ -1537,16 +1536,18 @@ struct move_only_holder_caster : public type_caster_base { explicit operator type*() { return this->value; } explicit operator type&() { return *(this->value); } - explicit operator holder_type*() { return std::addressof(holder); } // Workaround for Intel compiler bug // see pybind11 issue 94 - #if defined(__ICC) || defined(__INTEL_COMPILER) - operator holder_type&() { return holder; } - #else - explicit operator holder_type&() { return holder; } + #if !defined(__ICC) && !defined(__INTEL_COMPILER) + explicit #endif - explicit operator holder_type&&() { value = nullptr; return std::move(holder); } + operator holder_type&&() { + // TODO: release object instance in python + // clear_instance(src_handle->ptr()); ??? + + return std::move(*holder_ptr); + } static handle cast(const holder_type &src, return_value_policy, handle) { const auto *ptr = holder_helper::get(src); @@ -1562,8 +1563,8 @@ struct move_only_holder_caster : public type_caster_base { bool load_value(value_and_holder &&v_h) { if (v_h.holder_constructed()) { - value = v_h.value_ptr(); - holder = std::move(v_h.template holder()); + holder_ptr = std::addressof(v_h.template holder()); + value = const_cast(reinterpret_cast(holder_helper::get(*holder_ptr))); return true; } else { throw cast_error("Unable to cast from non-held to held instance (T& to Holder) " @@ -1579,22 +1580,13 @@ struct move_only_holder_caster : public type_caster_base { bool try_implicit_casts(handle, bool) { return false; } template ::value, int> = 0> - bool try_implicit_casts(handle src, bool convert) { - for (auto &cast : typeinfo->implicit_casts) { - move_only_holder_caster sub_caster(*cast.first); - if (sub_caster.load(src, convert)) { - value = cast.second(sub_caster.value); - holder = holder_type(sub_caster.holder, (type *) value); - return true; - } - } - return false; - } + bool try_implicit_casts(handle, bool) { return false; } static bool try_direct_conversions(handle) { return false; } - holder_type holder; + holder_type* holder_ptr = nullptr; + handle* src_handle = nullptr; }; template From e9e8cc474f5ec17eff17e1f3e67ede84ac71e50a Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Tue, 31 Dec 2019 00:02:41 +0100 Subject: [PATCH 8/9] remove unused cast operators --- include/pybind11/cast.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 90b6104138..a6c54801a6 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1534,9 +1534,6 @@ struct move_only_holder_caster : public type_caster_base { template using cast_op_type = detail::movable_cast_op_type; - explicit operator type*() { return this->value; } - explicit operator type&() { return *(this->value); } - // Workaround for Intel compiler bug // see pybind11 issue 94 #if !defined(__ICC) && !defined(__INTEL_COMPILER) From c61b0b73307ca0ff6a546944cd54d6d9f7d4ad81 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Wed, 1 Jan 2020 21:15:33 +0100 Subject: [PATCH 9/9] solved multiple access issue --- include/pybind11/cast.h | 30 +++++++++++------------------- tests/test_move_arg.cpp | 3 +++ tests/test_move_arg.py | 11 +++++++---- 3 files changed, 21 insertions(+), 23 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index a6c54801a6..a72bee78c8 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -567,23 +567,9 @@ class type_caster_generic { // Base methods for generic caster; there are overridden in copyable_holder_caster void load_value(value_and_holder &&v_h) { - auto *&vptr = v_h.value_ptr(); - // Lazy allocation for unallocated values: - if (vptr == nullptr) { - auto *type = v_h.type ? v_h.type : typeinfo; - if (type->operator_new) { - vptr = type->operator_new(type->type_size); - } else { - #if defined(PYBIND11_CPP17) - if (type->type_align > __STDCPP_DEFAULT_NEW_ALIGNMENT__) - vptr = ::operator new(type->type_size, - (std::align_val_t) type->type_align); - else - #endif - vptr = ::operator new(type->type_size); - } - } - value = vptr; + value = v_h.value_ptr(); + if (value == nullptr) + throw cast_error("Invalid object instance"); } bool try_implicit_casts(handle src, bool convert) { for (auto &cast : typeinfo->implicit_casts) { @@ -1528,7 +1514,7 @@ struct move_only_holder_caster : public type_caster_base { bool load(handle& src, bool convert) { bool success = base::template load_impl>(src, convert); if (success) // On success, remember src pointer to withdraw later - src_handle = std::addressof(src); + this->v_h = reinterpret_cast(src.ptr())->get_value_and_holder(); return success; } @@ -1540,6 +1526,10 @@ struct move_only_holder_caster : public type_caster_base { explicit #endif operator holder_type&&() { + // In load_value() value_ptr was still valid. If it's invalid now, another argument consumed the same value before. + if (!v_h.value_ptr()) + throw cast_error("Multiple access to moved argument"); + v_h.value_ptr() = nullptr; // TODO: release object instance in python // clear_instance(src_handle->ptr()); ??? @@ -1562,6 +1552,8 @@ struct move_only_holder_caster : public type_caster_base { if (v_h.holder_constructed()) { holder_ptr = std::addressof(v_h.template holder()); value = const_cast(reinterpret_cast(holder_helper::get(*holder_ptr))); + if (!value) + throw cast_error("Invalid object instance"); return true; } else { throw cast_error("Unable to cast from non-held to held instance (T& to Holder) " @@ -1583,7 +1575,7 @@ struct move_only_holder_caster : public type_caster_base { holder_type* holder_ptr = nullptr; - handle* src_handle = nullptr; + value_and_holder v_h; }; template diff --git a/tests/test_move_arg.cpp b/tests/test_move_arg.cpp index 1f60d4bff8..35d989cb3a 100644 --- a/tests/test_move_arg.cpp +++ b/tests/test_move_arg.cpp @@ -59,5 +59,8 @@ PYBIND11_MODULE(test_move_arg, m) { std::cout << " old: " << item.get() << "\n new: " << *sink.get() << "\n"; }, py::call_guard()); + m.def("consume_twice", [](my_ptr&& /*first*/, my_ptr&& /*second*/) { + }, py::call_guard()); + m.def("consume_str", [](std::string&& s) { std::string o(std::move(s)); }); } diff --git a/tests/test_move_arg.py b/tests/test_move_arg.py index ed319e17b7..1d4c425b05 100644 --- a/tests/test_move_arg.py +++ b/tests/test_move_arg.py @@ -10,14 +10,17 @@ def test(): del item print(other) -def test_produce(): +def test_consume(): item = Item(42) - access(item) consume(item) - access(item) + access(item) # should raise, because item is accessed after consumption + +def test_consume_twice(): + item = Item(42) + consume_twice(item, item) # should raise, because item is consumed twice def test_foo(): foo() if __name__ == "__main__": - test_produce() + test_consume()