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 assertion failure for unions (#1685) #1709

Merged
merged 1 commit into from
Jun 11, 2019
Merged

Conversation

rolandd
Copy link
Contributor

@rolandd rolandd commented Feb 24, 2019

In def_readonly and def_readwrite, there is an assertion that the member comes
from the class or a base class:

static_assert(std::is_base_of<C, type>::value, "...");

However, if C and type are the same type, is_base_of will still only be true
if they are the same non-union type. This means we can't define accessors
for the members of a union type because of this assertion.

Update the assertion to test

std::is_same<C, type>::value || std::is_base_of<C, type>::value

which will allow union types, or members of base classes.

Also add a basic unit test for accessing unions.

@rolandd
Copy link
Contributor Author

rolandd commented Feb 24, 2019

None of the automated test failures look related to my change. Most of the Travis CI tests passed, and the environments that failed seem like flakiness unrelated to my change; the AppVeyor build failures look like a configuration problem too.

@wjakob
Copy link
Member

wjakob commented Jun 10, 2019

Hi @rolandd,

apologies for the long delay-- could you rebase your commits to the latest pybind11 so that CI has a chance to re-run?

Thanks,
Wenzel

@rolandd
Copy link
Contributor Author

rolandd commented Jun 10, 2019

Done, just rebased on pybind11 master.

@rolandd
Copy link
Contributor Author

rolandd commented Jun 10, 2019

The travis style check failed with

Warning, treated as error:
/home/travis/build/pybind/pybind11/docs/advanced/classes.rst:328:malformed hyperlink target.
The command "$PY_CMD -m sphinx -W -b html docs docs/.build" exited with 2.

but my change doesn't touch that file.

In def_readonly and def_readwrite, there is an assertion that the member comes
from the class or a base class:

    static_assert(std::is_base_of<C, type>::value, "...");

However, if C and type are the same type, is_base_of will still only be true
if they are the same _non-union_ type.  This means we can't define accessors
for the members of a union type because of this assertion.

Update the assertion to test

    std::is_same<C, type>::value || std::is_base_of<C, type>::value

which will allow union types, or members of base classes.

Also add a basic unit test for accessing unions.
@wjakob
Copy link
Member

wjakob commented Jun 11, 2019

Fantastic, thank you!

@wjakob wjakob merged commit 8462dd7 into pybind:master Jun 11, 2019
wjakob pushed a commit that referenced this pull request Jun 11, 2019
In def_readonly and def_readwrite, there is an assertion that the member comes
from the class or a base class:

    static_assert(std::is_base_of<C, type>::value, "...");

However, if C and type are the same type, is_base_of will still only be true
if they are the same _non-union_ type.  This means we can't define accessors
for the members of a union type because of this assertion.

Update the assertion to test

    std::is_same<C, type>::value || std::is_base_of<C, type>::value

which will allow union types, or members of base classes.

Also add a basic unit test for accessing unions.
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

2 participants