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

Splitting out detail/type_caster_base.h from cast.h, with iwyu cleanup. #2841

Merged
merged 3 commits into from
Feb 23, 2021

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Feb 2, 2021

The first part of cast.h is moved to detail/type_caster_base.h.
The break point is the end of class type_caster_base.

I believe breaking up the very large cast.h is a good step in its own right, but the immediate motivation is to help moving PR #2672 forward.

Cleanup of includes was guided by include-what-you-use 0.12 based on clang version 9.0.1-11.
The approach was ad-hoc but simple:

echo '#include "cast.h"' > include/pybind11/cast.cc
echo '#include "type_caster_base.h"' > include/pybind11/detail/type_caster_base.cc
iwyu -std=c++17 -DPYBIND11_TEST_BOOST -Ipybind11/include -I/usr/include/python3.8 -I/usr/include/eigen3 include/pybind11/cast.cc
iwyu -std=c++17 -DPYBIND11_TEST_BOOST -Ipybind11/include -I/usr/include/python3.8 -I/usr/include/eigen3 include/pybind11/detail/type_caster_base.cc
  • The suggested fixes were applied manually, with pybind11 includes first, system includes second.
  • A few suggested forward declarations and warnings about missing inline functions were ignored. Resolving them probably needs refactoring of other files.
  • Suggested Python includes (such as patchlevel.h) were ignored.
  • A few missing std:: prefixes were added to avoid C-style includes.

Changelog not needed.

@rwgk
Copy link
Collaborator Author

rwgk commented Feb 2, 2021

Copy link
Collaborator

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

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

I see @wjakob was requested for review. That sounds like a good idea, indeed.

In general, this PR doesn't seem like a bad idea to me, though I'm slightly confused how it helps you on the other PR? Before, you would include pybind11/cast.h, now you include pybind11/detail/type_caster_base.h?

include/pybind11/cast.h Show resolved Hide resolved
@henryiii
Copy link
Collaborator

henryiii commented Feb 5, 2021

I'm generally in favor of more, smaller files with better separation. Makes it easier to refactor, find what you are looking for, include just the right part, etc.

The correct includes should be places in each file, though. Ideally include-what-you-use style.

@rwgk
Copy link
Collaborator Author

rwgk commented Feb 5, 2021

The correct includes should be places in each file, though. Ideally include-what-you-use style.

Thanks @henryiii, will do. In the meantime I made more progress running all existing unit tests with the new smart_holder as the default holder (in my "sideline" branch). I'll get back here either as soon as I get all the way through, or reach some stable intermediate point.

@rwgk rwgk changed the title Splitting out detail/type_caster_base.h from cast.h (NO other changes). Splitting out detail/type_caster_base.h from cast.h, with iwyu cleanup. Feb 22, 2021
Copy link
Member

@wjakob wjakob left a comment

Choose a reason for hiding this comment

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

LGTM!

@rwgk
Copy link
Collaborator Author

rwgk commented Feb 23, 2021

Thanks! :-)

@rwgk rwgk merged commit 0c42250 into pybind:master Feb 23, 2021
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Feb 23, 2021
@rwgk rwgk deleted the type_cast_base_h branch February 23, 2021 02:54
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Feb 23, 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.

4 participants