Skip to content

Shuffling code in test_multiple_inheritance.cpp to separate struct/class definitions from bindings code. #2890

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

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Mar 5, 2021

Similar to #2875 but significantly simpler.

  • Back-porting from smart_holder branch, to minimize diffs and the potential for merge conflicts. In particular, this PR is to help merge the smart_holder branch into the https://github.com/RobotLocomotion/pybind11 drake branch.
  • Mostly code is just shuffled (see below for proof that nothing got lost).
  • All code outside TEST_MODULE moved to the anonymous namespace. This isn't strictly necessary for the smart_holder work, but best practice.
  • A few comments were duplicated for clarity, to appear both in the anonymous namespace and in TEST_MODULE. See diff below.

Changelog not needed.

Simple proof that nothing got lost:

cd pybind11/tests
git checkout master
cat test_multiple_inheritance.cpp | sed 's/^ *//' | grep -v '^$' | sort > zmaster
git checkout test_multiple_inheritance_non_interleaved
cat test_multiple_inheritance.cpp | sed 's/^ *//' | grep -v '^$' | sort > zpr
diff zmaster zpr
rm zmaster zpr

The full output from the diff command above:

30a31
> // but implement `struct`s and `class`es in the anonymous namespace above.
97a99,101
> } // namespace
> namespace {
> // Please do not interleave `struct` and `class` definitions with bindings code,
172a177
> // test_mi_base_return
175a181
> // test_mi_unaligned_base
180a187
> // test_multiple_inheritance_virtbase
183a191
> // This helps keeping the smart_holder branch in sync with master.

@rwgk rwgk force-pushed the test_multiple_inheritance_non_interleaved branch from e616a41 to dee0255 Compare March 5, 2021 21:05
@EricCousineau-TRI EricCousineau-TRI self-assigned this Mar 5, 2021
@EricCousineau-TRI EricCousineau-TRI self-requested a review March 5, 2021 22:38
@EricCousineau-TRI EricCousineau-TRI removed their assignment Mar 5, 2021
Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Echoing @wjakob's sentiment in #2875 - looks straightforward, and it's always good to employ best practices!

@EricCousineau-TRI
Copy link
Collaborator

Ignoring stalled job on CI (see #2891); merging!

@EricCousineau-TRI EricCousineau-TRI merged commit 44678e5 into pybind:master Mar 5, 2021
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Mar 5, 2021
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Mar 5, 2021
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