Skip to content

Fix pybind11 interoperability with Clang trunk #1269

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

Merged
merged 1 commit into from
Feb 6, 2018
Merged

Conversation

wjakob
Copy link
Member

@wjakob wjakob commented Feb 5, 2018

pybind11 fails to compile on the latest Clang. There are two issues:

  1. #include <experimental/optional> now fails with a preprocessor error stating that #include <optional> should be used instead. The solution is to never include experimental/optional if optional is available.

  2. There are places in the pybind11 codebase that (indirectly) call make_tuple() without any arguments, e.g. the str_attr accessor. This causes am obscure compiler failure (excerpt below).

The underlying problem is that std::array<...>::operator[] can no longer be instantiated for arrays with zero elements. The solution is to provide a make_tuple() overload that takes no arguments.

In file included from ext/pybind11/include/pybind11/cast.h:17:
/usr/local/bin/../include/c++/v1/array:244:61: error: non-const lvalue reference to type 'std::__1::array<pybind11::object, 0>::value_type' (aka 'pybind11::object') cannot bind to a value of unrelated type 'std::__1::aligned_storage<8, 8>::type'                                             ake
    reference operator[](size_type __n)             {return __elems_[__n];}
                                                            ^~~~~~~~~~~~~
ext/pybind11/include/pybind11/cast.h:1696:14: note: in instantiation of member function 'std::__1::array<pybind11::object, 0>::operator[]' requested here
        if (!args[i]) {
             ^
ext/pybind11/include/pybind11/cast.h:1879:28: note: in instantiation of function template specialization 'pybind11::make_tuple<pybind11::return_value_policy::automatic_reference>' requested here
        : m_args(pybind11::make_tuple<policy>(std::forward<Ts>(values)...)) { }                                 ba/python/python.h:8:
                           ^
ext/pybind11/include/pybind11/cast.h:2023:12: note: in instantiation of function template specialization 'pybind11::detail::simple_collector<pybind11::return_value_policy::automatic_reference>::simple_collector<>' requested here
    return simple_collector<policy>(std::forward<Args>(args)...);
           ^
ext/pybind11/include/pybind11/cast.h:2043:20: note: in instantiation of function template specialization 'pybind11::detail::collect_arguments<pybind11::return_value_policy::automatic_reference, void>' requested here
    return detail::collect_arguments<policy>(std::forward<Args>(args)...).call(derived().ptr());
                   ^
ext/pybind11/include/pybind11/pybind11.h:200:58: note: in instantiation of function template specialization 'pybind11::detail::object_api<pybind11::detail::accessor<pybind11::detail::accessor_policies::str_attr> >::operator()<pybind11::return_value_policy::automatic_reference>' requested here
                a.descr = strdup(a.value.attr("__repr__")().cast<std::string>().c_str());     

@wjakob
Copy link
Member Author

wjakob commented Feb 5, 2018

@jagerman -- does this look good to you? I hope to merge this fairly soon, as it is blocking CI for another project.

@wjakob
Copy link
Member Author

wjakob commented Feb 6, 2018

I forgot to mention: the <optional>-related issue appears when including `pybind11/stl.h``.

@jagerman
Copy link
Member

jagerman commented Feb 6, 2018

The std::optional code is getting messy: we aren't allowed to include <optional> in C++14 mode, but now also can't load <experimental/optional> in C++14 mode (at least under clang).

Maybe we should just strip out the <experimental/optional> support completely? If someone is using an old compiler that requires it, they can always add it back in, but I don't think pybind should keep supporting it indefinitely.

Also: does this happen under clang 6? I've been meaning to update the clang c++17 build from 5 to 6 now that it has hit RC.

@jagerman
Copy link
Member

jagerman commented Feb 6, 2018

It seems llvm-toolchain-trusty-6.0 isn't a trusted travis-ci repo yet (travis-ci/apt-source-safelist#373), so scratch that.

The commit looks fine to me. I'll pull it into the 2.2.2 PR as well.

@jagerman
Copy link
Member

jagerman commented Feb 6, 2018

On second thought: we can worry later about whether or not to pull it out of master--the fixes here look good to me. I've cherry-picked it into the 2.2.2 (PR #1250) proposed branch as well. (I think we should release that soon; there's quite a few small fixes in it).

@wjakob wjakob merged commit ff6bd09 into pybind:master Feb 6, 2018
@wjakob
Copy link
Member Author

wjakob commented Feb 6, 2018

All agreed about the somewhat messy experimental/optional support, I'd be fine with axing it in a future v3.0.

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.

2 participants