From 46dbee7d423f44bbf1f8a216a6f6bb1bf85b95b5 Mon Sep 17 00:00:00 2001 From: Bruce Merry Date: Fri, 26 May 2017 08:52:54 +0200 Subject: [PATCH] Avoid explicitly resetting a std::[experimental::]optional Now that #851 has removed all multiple uses of a caster, it can just use the default-constructed value with needing a reset. This fixes two issues: 1. With std::experimental::optional (at least under GCC 5.4), the `= {}` would construct an instance of the optional type and then move-assign it, which fails if the value type isn't move-assignable. 2. With older versions of Boost, the `= {}` could fail because it is ambiguous, allowing construction of either `boost::none` or the value type. --- include/pybind11/stl.h | 3 +-- tests/test_python_types.cpp | 24 ++++++++++++++++++++++++ tests/test_python_types.py | 16 ++++++++++++++-- 3 files changed, 39 insertions(+), 4 deletions(-) diff --git a/include/pybind11/stl.h b/include/pybind11/stl.h index 495f85d3..3d38b539 100644 --- a/include/pybind11/stl.h +++ b/include/pybind11/stl.h @@ -242,8 +242,7 @@ template struct optional_caster { if (!src) { return false; } else if (src.is_none()) { - value = {}; // nullopt - return true; + return true; // default-constructed value is already empty } value_conv inner_caster; if (!inner_caster.load(src, convert)) diff --git a/tests/test_python_types.cpp b/tests/test_python_types.cpp index f9bda617..f3375c67 100644 --- a/tests/test_python_types.cpp +++ b/tests/test_python_types.cpp @@ -188,6 +188,18 @@ struct MoveOutContainer { struct UnregisteredType { }; +// Class that can be move- and copy-constructed, but not assigned +struct NoAssign { + int value; + + explicit NoAssign(int value = 0) : value(value) {} + NoAssign(const NoAssign &) = default; + NoAssign(NoAssign &&) = default; + + NoAssign &operator=(const NoAssign &) = delete; + NoAssign &operator=(NoAssign &&) = delete; +}; + test_initializer python_types([](py::module &m) { /* No constructor is explicitly defined below. An exception is raised when trying to construct it directly from Python */ @@ -236,6 +248,10 @@ test_initializer python_types([](py::module &m) { py::print("{a} + {b} = {c}"_s.format("a"_a="py::print", "b"_a="str.format", "c"_a="this")); }); + py::class_(m, "NoAssign", "Class with no C++ assignment operators") + .def(py::init<>()) + .def(py::init()); + m.def("test_print_failure", []() { py::print(42, UnregisteredType()); }); #if !defined(NDEBUG) m.attr("debug_enabled") = true; @@ -326,6 +342,7 @@ test_initializer python_types([](py::module &m) { #ifdef PYBIND11_HAS_OPTIONAL has_optional = true; using opt_int = std::optional; + using opt_no_assign = std::optional; m.def("double_or_zero", [](const opt_int& x) -> int { return x.value_or(0) * 2; }); @@ -335,11 +352,15 @@ test_initializer python_types([](py::module &m) { m.def("test_nullopt", [](opt_int x) { return x.value_or(42); }, py::arg_v("x", std::nullopt, "None")); + m.def("test_no_assign", [](const opt_no_assign &x) { + return x ? x->value : 42; + }, py::arg_v("x", std::nullopt, "None")); #endif #ifdef PYBIND11_HAS_EXP_OPTIONAL has_exp_optional = true; using exp_opt_int = std::experimental::optional; + using exp_opt_no_assign = std::experimental::optional; m.def("double_or_zero_exp", [](const exp_opt_int& x) -> int { return x.value_or(0) * 2; }); @@ -349,6 +370,9 @@ test_initializer python_types([](py::module &m) { m.def("test_nullopt_exp", [](exp_opt_int x) { return x.value_or(42); }, py::arg_v("x", std::experimental::nullopt, "None")); + m.def("test_no_assign_exp", [](const exp_opt_no_assign &x) { + return x ? x->value : 42; + }, py::arg_v("x", std::experimental::nullopt, "None")); #endif m.attr("has_optional") = has_optional; diff --git a/tests/test_python_types.py b/tests/test_python_types.py index e427c467..f7c3d555 100644 --- a/tests/test_python_types.py +++ b/tests/test_python_types.py @@ -337,7 +337,8 @@ def test_accessors(): @pytest.mark.skipif(not has_optional, reason='no ') def test_optional(): - from pybind11_tests import double_or_zero, half_or_none, test_nullopt + from pybind11_tests import (double_or_zero, half_or_none, test_nullopt, + test_no_assign, NoAssign) assert double_or_zero(None) == 0 assert double_or_zero(42) == 84 @@ -352,10 +353,16 @@ def test_optional(): assert test_nullopt(42) == 42 assert test_nullopt(43) == 43 + assert test_no_assign() == 42 + assert test_no_assign(None) == 42 + assert test_no_assign(NoAssign(43)) == 43 + pytest.raises(TypeError, test_no_assign, 43) + @pytest.mark.skipif(not has_exp_optional, reason='no ') def test_exp_optional(): - from pybind11_tests import double_or_zero_exp, half_or_none_exp, test_nullopt_exp + from pybind11_tests import (double_or_zero_exp, half_or_none_exp, test_nullopt_exp, + test_no_assign_exp, NoAssign) assert double_or_zero_exp(None) == 0 assert double_or_zero_exp(42) == 84 @@ -370,6 +377,11 @@ def test_exp_optional(): assert test_nullopt_exp(42) == 42 assert test_nullopt_exp(43) == 43 + assert test_no_assign_exp() == 42 + assert test_no_assign_exp(None) == 42 + assert test_no_assign_exp(NoAssign(43)) == 43 + pytest.raises(TypeError, test_no_assign_exp, 43) + @pytest.mark.skipif(not hasattr(pybind11_tests, "load_variant"), reason='no ') def test_variant(doc):