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

Add static_assert to holder casters #786

Merged
merged 1 commit into from
Apr 8, 2017

Conversation

jagerman
Copy link
Member

@jagerman jagerman commented Apr 7, 2017

The holder casters assume but don't check that a holder<type>'s type is really a type_caster_base<type>; this adds a static_assert to make sure this is really the case, to turn things like std::shared_ptr<array> into a compilation failure.

Fixes #785

There is some merit to making the holder casters use a proper type_caster<type> rather than a type_caster_base<type> so that you could, for example, have a std::shared_ptr<std::string> or std::unique_ptr<Eigen::MatrixXd>. With this commit, holder arguments around non-generic type casters fails at compile time; before this commit, those failures show up in strange runtime failures (due to invoking the generic type caster rather than the specialized type caster).

The holder casters assume but don't check that a `holder<type>`'s `type`
is really a `type_caster_base<type>`; this adds a static_assert to make
sure this is really the case, to turn things like
`std::shared_ptr<array>` into a compilation failure.

Fixes pybind#785
@jagerman
Copy link
Member Author

jagerman commented Apr 7, 2017

Just to be clear: I did not change this to use proper type casters because it would require a fairly substantial rewrite of the holder casters, which are currently quite closely tied to type_caster_base<type> internals.

@wjakob
Copy link
Member

wjakob commented Apr 7, 2017

+1 That seems like a really nice way to catch this early and avoid confusion.

@jagerman
Copy link
Member Author

jagerman commented Apr 7, 2017

Actually I don't think this really solves #785; this just changes the error to a compile-time failure. We're still unable to handle a holder around a type with a custom converter, which I think is going to happen in #785 when he switches to composition rather than inheritance.

@jagerman
Copy link
Member Author

jagerman commented Apr 8, 2017

I'm going to merge this to take care of the current internal error issue; I opened #787 for the second part of this.

@jagerman jagerman merged commit 501135f into pybind:master Apr 8, 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