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

def_readwrite/readonly: bind using class type, not inferred type #911

Closed
wants to merge 1 commit into from

Conversation

jagerman
Copy link
Member

@jagerman jagerman commented Jun 20, 2017

Current def_readwrite/def_readonly create getter/setter lambdas that
take the inferred type C as the first argument, but this is a problem
if the member is actually a base class member (e.g.

derived.def_readonly("x", &Base::x)

because this ends up creating a setter/getter that aren't callable with
a derived instance when Base isn't registered: it invariably produces
an invalid argument failure when attempting to access the property due
to the D instance/B argument mismatch.

This commit changes the generated lambdas to use the type (as given in
class_<type>) rather than the inferred C for the lambdas, and adds a
static_assert failure for an attempt to call it with a member that isn't
part of type (or a base class).

Fixes #910

Current def_readwrite/def_readonly create getter/setter lambda that
take the inferred type `C` as the first argument, but this is a problem
if the member is actually a base class member (e.g.

    derived.def_readonly("x", &Base::x)

because this ends up creating a setter/getter that aren't callable with
a derived instance when `Base` isn't registered: it invariably produces
an invalid argument failure when attempting to access the property due
to the `D` instance/`B` argument mismatch.

This commit changes the generated lambdas to use the `type` (as given in
`class_<type>`) rather than the inferred `C` for the lambdas, and adds a
static_assert failure for an attempt to call it with a member that isn't
part of `type` (or a base class).
@bmerry
Copy link
Contributor

bmerry commented Jun 23, 2017

Isn't this already fixed by #855?

@jagerman
Copy link
Member Author

Yes, if #855 is merged it fixes this as well. There is some concern than #855 is too complicated, however, so if we decide to drop it, this is a partial alternative.

@jagerman
Copy link
Member Author

I've simplified #855; closing this in favour of it.

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