From c3d81d235fda1240fcb9bbe9c9ac1bbac9dc59f2 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Thu, 11 Jan 2018 13:22:13 -0400 Subject: [PATCH] Use stricter brace initialization This updates the `py::init` constructors to only use brace initialization for aggregate initiailization if there is no constructor with the given arguments. This, in particular, fixes the regression in #1247 where the presence of a `std::initializer_list` constructor started being invoked for constructor invocations in 2.2 even when there was a specific constructor of the desired type. The added test case demonstrates: without this change, it fails to compile because the `.def(py::init>())` constructor tries to invoke the `T(std::initializer_list>)` constructor rather than the `T(std::vector)` constructor. By only using `new T{...}`-style construction when a `T(...)` constructor doesn't exist, we should bypass this by while still allowing `py::init<...>` to be used for aggregate type initialization (since such types, by definition, don't have a user-declared constructor). --- include/pybind11/detail/init.h | 20 +++++++++++++++----- tests/test_class.cpp | 16 ++++++++++++++++ tests/test_class.py | 6 ++++++ 3 files changed, 37 insertions(+), 5 deletions(-) diff --git a/include/pybind11/detail/init.h b/include/pybind11/detail/init.h index c3594a190..82f740760 100644 --- a/include/pybind11/detail/init.h +++ b/include/pybind11/detail/init.h @@ -52,6 +52,16 @@ bool is_alias(Cpp *ptr) { template constexpr bool is_alias(void *) { return false; } +// Constructs and returns a new object; if the given arguments don't map to a constructor, we fall +// back to brace aggregate initiailization so that for aggregate initialization can be used with +// py::init, e.g. `py::init` to initialize a `struct T { int a; int b; }`. For +// non-aggregate types, we need to use an ordinary T(...) constructor (invoking as `T{...}` usually +// works, but will not do the expected thing when `T` has an `initializer_list` constructor). +template ::value, int> = 0> +inline Class *construct_or_initialize(Args &&...args) { return new Class(std::forward(args)...); } +template ::value, int> = 0> +inline Class *construct_or_initialize(Args &&...args) { return new Class{std::forward(args)...}; } + // Attempts to constructs an alias using a `Alias(Cpp &&)` constructor. This allows types with // an alias to provide only a single Cpp factory function as long as the Alias can be // constructed from an rvalue reference of the base Cpp type. This means that Alias classes @@ -161,7 +171,7 @@ struct constructor { template = 0> static void execute(Class &cl, const Extra&... extra) { cl.def("__init__", [](value_and_holder &v_h, Args... args) { - v_h.value_ptr() = new Cpp{std::forward(args)...}; + v_h.value_ptr() = construct_or_initialize>(std::forward(args)...); }, is_new_style_constructor(), extra...); } @@ -171,9 +181,9 @@ struct constructor { static void execute(Class &cl, const Extra&... extra) { cl.def("__init__", [](value_and_holder &v_h, Args... args) { if (Py_TYPE(v_h.inst) == v_h.type->type) - v_h.value_ptr() = new Cpp{std::forward(args)...}; + v_h.value_ptr() = construct_or_initialize>(std::forward(args)...); else - v_h.value_ptr() = new Alias{std::forward(args)...}; + v_h.value_ptr() = construct_or_initialize>(std::forward(args)...); }, is_new_style_constructor(), extra...); } @@ -182,7 +192,7 @@ struct constructor { !std::is_constructible, Args...>::value, int> = 0> static void execute(Class &cl, const Extra&... extra) { cl.def("__init__", [](value_and_holder &v_h, Args... args) { - v_h.value_ptr() = new Alias{std::forward(args)...}; + v_h.value_ptr() = construct_or_initialize>(std::forward(args)...); }, is_new_style_constructor(), extra...); } }; @@ -193,7 +203,7 @@ template struct alias_constructor { enable_if_t, Args...>::value, int> = 0> static void execute(Class &cl, const Extra&... extra) { cl.def("__init__", [](value_and_holder &v_h, Args... args) { - v_h.value_ptr() = new Alias{std::forward(args)...}; + v_h.value_ptr() = construct_or_initialize>(std::forward(args)...); }, is_new_style_constructor(), extra...); } }; diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 927adf3f9..f0b5873df 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -10,6 +10,16 @@ #include "pybind11_tests.h" #include "constructor_stats.h" #include "local_bindings.h" +#include + +// test_brace_initialization +struct NoBraceInitialization { + NoBraceInitialization(std::vector v) : vec{std::move(v)} {} + template + NoBraceInitialization(std::initializer_list l) : vec(l) {} + + std::vector vec; +}; TEST_SUBMODULE(class_, m) { // test_instance @@ -291,6 +301,12 @@ TEST_SUBMODULE(class_, m) { .def(py::init()) .def_readwrite("field1", &BraceInitialization::field1) .def_readwrite("field2", &BraceInitialization::field2); + // We *don't* want to construct using braces when the given constructor argument maps to a + // constructor, because brace initialization could go to the wrong place (in particular when + // there is also an `initializer_list`-accept constructor): + py::class_(m, "NoBraceInitialization") + .def(py::init>()) + .def_readonly("vec", &NoBraceInitialization::vec); // test_reentrant_implicit_conversion_failure // #1035: issue with runaway reentrant implicit conversion diff --git a/tests/test_class.py b/tests/test_class.py index d94b61bb3..8cf4757cb 100644 --- a/tests/test_class.py +++ b/tests/test_class.py @@ -228,6 +228,12 @@ def test_brace_initialization(): assert a.field1 == 123 assert a.field2 == "test" + # Tests that a non-simple class doesn't get brace initialization (if the + # class defines an initializer_list constructor, in particular, it would + # win over the expected constructor). + b = m.NoBraceInitialization([123, 456]) + assert b.vec == [123, 456] + @pytest.unsupported_on_pypy def test_class_refcount():