Skip to content

Commit 46dbee7

Browse files
bmerrydean0x7d
authored andcommitted
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.
1 parent 4f9ee6e commit 46dbee7

File tree

3 files changed

+39
-4
lines changed

3 files changed

+39
-4
lines changed

include/pybind11/stl.h

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -242,8 +242,7 @@ template<typename T> struct optional_caster {
242242
if (!src) {
243243
return false;
244244
} else if (src.is_none()) {
245-
value = {}; // nullopt
246-
return true;
245+
return true; // default-constructed value is already empty
247246
}
248247
value_conv inner_caster;
249248
if (!inner_caster.load(src, convert))

tests/test_python_types.cpp

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,18 @@ struct MoveOutContainer {
188188

189189
struct UnregisteredType { };
190190

191+
// Class that can be move- and copy-constructed, but not assigned
192+
struct NoAssign {
193+
int value;
194+
195+
explicit NoAssign(int value = 0) : value(value) {}
196+
NoAssign(const NoAssign &) = default;
197+
NoAssign(NoAssign &&) = default;
198+
199+
NoAssign &operator=(const NoAssign &) = delete;
200+
NoAssign &operator=(NoAssign &&) = delete;
201+
};
202+
191203
test_initializer python_types([](py::module &m) {
192204
/* No constructor is explicitly defined below. An exception is raised when
193205
trying to construct it directly from Python */
@@ -236,6 +248,10 @@ test_initializer python_types([](py::module &m) {
236248
py::print("{a} + {b} = {c}"_s.format("a"_a="py::print", "b"_a="str.format", "c"_a="this"));
237249
});
238250

251+
py::class_<NoAssign>(m, "NoAssign", "Class with no C++ assignment operators")
252+
.def(py::init<>())
253+
.def(py::init<int>());
254+
239255
m.def("test_print_failure", []() { py::print(42, UnregisteredType()); });
240256
#if !defined(NDEBUG)
241257
m.attr("debug_enabled") = true;
@@ -326,6 +342,7 @@ test_initializer python_types([](py::module &m) {
326342
#ifdef PYBIND11_HAS_OPTIONAL
327343
has_optional = true;
328344
using opt_int = std::optional<int>;
345+
using opt_no_assign = std::optional<NoAssign>;
329346
m.def("double_or_zero", [](const opt_int& x) -> int {
330347
return x.value_or(0) * 2;
331348
});
@@ -335,11 +352,15 @@ test_initializer python_types([](py::module &m) {
335352
m.def("test_nullopt", [](opt_int x) {
336353
return x.value_or(42);
337354
}, py::arg_v("x", std::nullopt, "None"));
355+
m.def("test_no_assign", [](const opt_no_assign &x) {
356+
return x ? x->value : 42;
357+
}, py::arg_v("x", std::nullopt, "None"));
338358
#endif
339359

340360
#ifdef PYBIND11_HAS_EXP_OPTIONAL
341361
has_exp_optional = true;
342362
using exp_opt_int = std::experimental::optional<int>;
363+
using exp_opt_no_assign = std::experimental::optional<NoAssign>;
343364
m.def("double_or_zero_exp", [](const exp_opt_int& x) -> int {
344365
return x.value_or(0) * 2;
345366
});
@@ -349,6 +370,9 @@ test_initializer python_types([](py::module &m) {
349370
m.def("test_nullopt_exp", [](exp_opt_int x) {
350371
return x.value_or(42);
351372
}, py::arg_v("x", std::experimental::nullopt, "None"));
373+
m.def("test_no_assign_exp", [](const exp_opt_no_assign &x) {
374+
return x ? x->value : 42;
375+
}, py::arg_v("x", std::experimental::nullopt, "None"));
352376
#endif
353377

354378
m.attr("has_optional") = has_optional;

tests/test_python_types.py

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -337,7 +337,8 @@ def func(self, x, *args):
337337

338338
@pytest.mark.skipif(not has_optional, reason='no <optional>')
339339
def test_optional():
340-
from pybind11_tests import double_or_zero, half_or_none, test_nullopt
340+
from pybind11_tests import (double_or_zero, half_or_none, test_nullopt,
341+
test_no_assign, NoAssign)
341342

342343
assert double_or_zero(None) == 0
343344
assert double_or_zero(42) == 84
@@ -352,10 +353,16 @@ def test_optional():
352353
assert test_nullopt(42) == 42
353354
assert test_nullopt(43) == 43
354355

356+
assert test_no_assign() == 42
357+
assert test_no_assign(None) == 42
358+
assert test_no_assign(NoAssign(43)) == 43
359+
pytest.raises(TypeError, test_no_assign, 43)
360+
355361

356362
@pytest.mark.skipif(not has_exp_optional, reason='no <experimental/optional>')
357363
def test_exp_optional():
358-
from pybind11_tests import double_or_zero_exp, half_or_none_exp, test_nullopt_exp
364+
from pybind11_tests import (double_or_zero_exp, half_or_none_exp, test_nullopt_exp,
365+
test_no_assign_exp, NoAssign)
359366

360367
assert double_or_zero_exp(None) == 0
361368
assert double_or_zero_exp(42) == 84
@@ -370,6 +377,11 @@ def test_exp_optional():
370377
assert test_nullopt_exp(42) == 42
371378
assert test_nullopt_exp(43) == 43
372379

380+
assert test_no_assign_exp() == 42
381+
assert test_no_assign_exp(None) == 42
382+
assert test_no_assign_exp(NoAssign(43)) == 43
383+
pytest.raises(TypeError, test_no_assign_exp, 43)
384+
373385

374386
@pytest.mark.skipif(not hasattr(pybind11_tests, "load_variant"), reason='no <variant>')
375387
def test_variant(doc):

0 commit comments

Comments
 (0)