Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Brace initialization for simple POD classes #1015

Merged
merged 2 commits into from
Aug 22, 2017
Merged

Conversation

wjakob
Copy link
Member

@wjakob wjakob commented Aug 21, 2017

For simple POD classes storing just a few fields, I often don't bother writing constructors and use the generalized C++11 brace initialization syntax. pybind11 doesn't like that and currently requires one to add constructors to the POD class (the py::init details use the C++03 syntax). This commit switches the pybind11 implementation to use brace initialization as well.

^@jagerman
^@dean0x7d
Unrelated: I almost did a force push to the master branch instead of my personal repo but noticed in the last moment. I switched the master branch to protected status now (i.e. force pushes not allowed) to prevent accidents -- it's easily removed temporarily if we ever need to.

Copy link
Member

@dean0x7d dean0x7d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Making simple structs simpler.

This is a minor corner case, but since initializer_list takes priority over regular constructors when brace initialization is used (the std::vector<int> issue), somebody somewhere may be affected when upgrading. Might be worth mentioning in the docs.

@@ -229,6 +229,16 @@ TEST_SUBMODULE(class_, m) {
// This test is actually part of test_local_bindings (test_duplicate_local), but we need a
// definition in a different compilation unit within the same module:
bind_local<LocalExternal, 17>(m, "LocalExternal", py::module_local());

struct BraceInitialization {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick comment above this line: // test_brace_initializaton

Having the name of the Python test function as a comment above the corresponding C++ code makes it very convenient to navigate from a failed test function to the C++ source.

@wjakob
Copy link
Member Author

wjakob commented Aug 21, 2017

done.

@jagerman
Copy link
Member

Unrelated: I almost did a force push to the master branch instead of my personal repo but noticed in the last moment.

I deliberately keep the pybind11/master remote named locally as upstream (really anything other than master will work fine). The main advantage is that git won't push to it by default because of the branch naming mismatch, instead giving the error:

fatal: The upstream branch of your current branch does not match
the name of your current branch.  To push to the upstream branch
on the remote, use

    git push upstream HEAD:master

To push to the branch of the same name on the remote, use

    git push upstream upstream

so that I can still push it using the first suggested command, but it's always an extra safety step (even against ordinary pushes, not just forced pushes).

.def(py::init<int, const std::string &>());

Note that brace initialization preferentially invokes constructor overloads
taking a ``std::initializer_list``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps add:

In the rare event that this causes an issue, you can work around it by using py::init(...) with a lambda function that constructs the new object as desired.

Since that makes it more than just a single sentence, it's probably useful to put it in a note section.

@jagerman
Copy link
Member

One small suggested addition to the docs, but otherwise this LGTM.


.. code-block:: cpp

struct POD {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a POD (std::string is not trivially copyable). Aggregate should be the term for this kind of structure and brace initialization.

@wjakob
Copy link
Member Author

wjakob commented Aug 22, 2017

Rebased and squashed down into 2 commits.

Unrelated: I noticed a number of warnings when compiling the test suite on Clang/OSX -- at least one of them is related to the new factory constructors.

[9/9/17] Building CXX object tests/CMakeFiles/pybind11_tests.dir/test_factory_constructors.cpp.o
/Users/wjakob/projects/pybind11/tests/test_factory_constructors.cpp:180:17: warning: lambda capture 'c' is not used [-Wunused-lambda-capture]
    auto c4a = [c](pointer_tag, TF4_tag, int a) { return new TestFactory4(a);};
                ^
1 warning generated.
[7/9/19] Building CXX object tests/CMakeFiles/pybind11_tests.dir/test_numpy_array.cpp.o
In file included from /Users/wjakob/projects/pybind11/tests/test_numpy_array.cpp:12:
include/pybind11/numpy.h:394:40: warning: implicit conversion changes signedness: 'long' to 'unsigned long' [-Wsign-conversion]
        static_assert(sizeof...(Ix) == Dims || Dynamic,
                                    ~~ ^~~~
/Users/wjakob/projects/pybind11/tests/test_numpy_array.cpp:239:18: note: in instantiation of function template specialization 'pybind11::detail::unchecked_mutable_reference<double, -1>::operator()<long, long>' requested here
                r(i, j) += v;
                 ^
In file included from /Users/wjakob/projects/pybind11/tests/test_numpy_array.cpp:12:
include/pybind11/numpy.h:345:40: warning: implicit conversion changes signedness: 'long' to 'unsigned long' [-Wsign-conversion]
        static_assert(sizeof...(Ix) == Dims || Dynamic,
                                    ~~ ^~~~
include/pybind11/numpy.h:396:43: note: in instantiation of function template specialization 'pybind11::detail::unchecked_reference<double, -1>::operator()<long, long>' requested here
        return const_cast<T &>(ConstBase::operator()(index...));
                                          ^
/Users/wjakob/projects/pybind11/tests/test_numpy_array.cpp:239:18: note: in instantiation of function template specialization 'pybind11::detail::unchecked_mutable_reference<double, -1>::operator()<long, long>' requested here
                r(i, j) += v;
                 ^
In file included from /Users/wjakob/projects/pybind11/tests/test_numpy_array.cpp:12:
include/pybind11/numpy.h:394:40: warning: implicit conversion changes signedness: 'long' to 'unsigned long' [-Wsign-conversion]
        static_assert(sizeof...(Ix) == Dims || Dynamic,
                                    ~~ ^~~~
/Users/wjakob/projects/pybind11/tests/test_numpy_array.cpp:248:14: note: in instantiation of function template specialization 'pybind11::detail::unchecked_mutable_reference<double, -1>::operator()<long, long, long>' requested here
            r(i, j, k) = start++;
             ^
In file included from /Users/wjakob/projects/pybind11/tests/test_numpy_array.cpp:12:
include/pybind11/numpy.h:345:40: warning: implicit conversion changes signedness: 'long' to 'unsigned long' [-Wsign-conversion]
        static_assert(sizeof...(Ix) == Dims || Dynamic,
                                    ~~ ^~~~
include/pybind11/numpy.h:396:43: note: in instantiation of function template specialization 'pybind11::detail::unchecked_reference<double, -1>::operator()<long, long, long>' requested here
        return const_cast<T &>(ConstBase::operator()(index...));
                                          ^
/Users/wjakob/projects/pybind11/tests/test_numpy_array.cpp:248:14: note: in instantiation of function template specialization 'pybind11::detail::unchecked_mutable_reference<double, -1>::operator()<long, long, long>' requested here
            r(i, j, k) = start++;

@dean0x7d
Copy link
Member

Unrelated: I noticed a number of warnings when compiling the test suite on Clang/OSX

Which version of clang? I'm not seeing these with appleclang v8.1.0. (In fact, seems to be reporting unknown warning option '-Wunused-lambda-capture').

@wjakob
Copy link
Member Author

wjakob commented Aug 22, 2017

It's clang trunk compiled from SVN :).

@wjakob wjakob merged commit 4336a7d into pybind:master Aug 22, 2017
@wjakob
Copy link
Member Author

wjakob commented Aug 22, 2017

merged.

@dean0x7d dean0x7d modified the milestone: v2.2 Aug 23, 2017
wjakob added a commit that referenced this pull request Aug 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants