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

Fix ambiguous initialize_list arguments #822

Merged
merged 1 commit into from
Apr 28, 2017

Conversation

jagerman
Copy link
Member

This removes the convert-from-arithemtic-scalar constructor of any_container added in #788 as it can result in ambiguous calls, as in:

    py::array_t<float>({ 1, 2 })

which could be intepreted as either of:

    py::array_t<float>(py::array_t<float>(1, 2))
    py::array_t<float>(py::detail::any_container({ 1, 2 }))

Removing the convert-from-arithmetic constructor reduces the number of implicit conversions, avoiding the ambiguity for array and array_t. This also re-adds the array/array_t constructors taking a scalar argument for backwards compatibility.

This removes the convert-from-arithemtic-scalar constructor of
any_container as it can result in ambiguous calls, as in:

    py::array_t<float>({ 1, 2 })

which could be intepreted as either of:

    py::array_t<float>(py::array_t<float>(1, 2))
    py::array_t<float>(py::detail::any_container({ 1, 2 }))

Removing the convert-from-arithmetic constructor reduces the number of
implicit conversions, avoiding the ambiguity for array and array_t.
This also re-adds the array/array_t constructors taking a scalar
argument for backwards compatibility.
@jagerman
Copy link
Member Author

jagerman commented Apr 28, 2017

@wjakob - I think this should fix the ambiguity issue; can you give it a try with your various (unmodified) projects to confirm?

(A related idea: it would be very useful to have some failure-allowed travis-ci builds that install pybind (without compiling the test suite since that's already done in the other builds), then clones a few open source pybind-using projects and tries to build and/or test them. It could help catching some backwards compatibility issues like this.)

@wjakob
Copy link
Member

wjakob commented Apr 28, 2017

Thanks! I confirm that this PR fixes the issue.

Regarding your suggestion: I think that our test suite is actually pretty good; it's gotten increasingly rare that something slips through. Downsides of testing further projects are that this would likely consume quite a few extra cycles -- not so nice given that we're on the free tier of Travis CI. My impression is that the previous approach of leaving enough stabilization time to catch rare regressions before releases suffices, though we can certainly discuss this further.

@jagerman
Copy link
Member Author

That's a good point, and given that we almost never remove things from the test suite, the coverage is probably pretty good.

@jagerman jagerman merged commit 51d18aa into pybind:master Apr 28, 2017
@dean0x7d dean0x7d modified the milestone: v2.2 Aug 13, 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.

3 participants