Skip to content

Move internal headers into detail subdirectory #1001

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

Merged
merged 1 commit into from
Aug 17, 2017

Conversation

dean0x7d
Copy link
Member

The header reorganization discussed in #708 can be broken up into 3 main parts:

  1. Moving internal headers into a detail subdirectory.
  2. Breaking up large detail-heavy headers into smaller ones (e.g. detail/meta.h for the metaprogramming bits).
  3. Separate cast subdirectory for type casters.

Instead of tackling all this at the same time, it would be best to take it in different stages. This PR takes care of number 1 by simply moving the non-user-facing headers (everything fully intact and no user-side changes). This would allow new features like #805 to already make use of the detail subdirectory.

Number 2 can be done separately in smaller pieces. Number 3 would have some user-facing changes and it would be best to do in conjunction with the caster changes from #864.

@jagerman
Copy link
Member

Are you proposing this first step for 2.2? I can update #805 to match.

@dean0x7d
Copy link
Member Author

dean0x7d commented Aug 13, 2017

Are you proposing this first step for 2.2? I can update #805 to match.

Yes. Forgot to mention that. Step 1 for v2.2 and steps 2 and 3 for v2.3.

@jagerman
Copy link
Member

Sounds good to me.

@@ -55,7 +56,6 @@ set(PYBIND11_HEADERS
include/pybind11/pytypes.h
include/pybind11/stl.h
include/pybind11/stl_bind.h
include/pybind11/typeid.h
Copy link
Member

Choose a reason for hiding this comment

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

typeid.h does provide the non-detail namespace py::type_id<T>(), which is a fairly useful function and might be in use. Maybe this one should be left for later?

Copy link
Member Author

Choose a reason for hiding this comment

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

My thinking here was that typeid.h is included anyway by cast.h and in turn by pybind11.h, so there's no way to lose out on py::type_id<T>() unless someone is using it without anything else from pybind11. But I wouldn't mind reverting it if I'm overlooking something.

Copy link
Member

Choose a reason for hiding this comment

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

That's true; it's fine with me to include it in the move. (Maybe type_id() should be moved out into cast.h or somewhere else, but that's not really a big deal—and as an undocumented mostly internal function it was arguably misplaced by not being in detail in the first place).

@@ -1,5 +1,5 @@
/*
pybind11/typeid.h: Convenience wrapper classes for basic Python types
pybind11/pytypes.h: Convenience wrapper classes for basic Python types
Copy link
Member

Choose a reason for hiding this comment

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

Good catch 👍

@dean0x7d dean0x7d added this to the v2.2 milestone Aug 14, 2017
jagerman added a commit to jagerman/pybind11 that referenced this pull request Aug 14, 2017
jagerman added a commit to jagerman/pybind11 that referenced this pull request Aug 14, 2017
@dean0x7d dean0x7d merged commit f580649 into pybind:master Aug 17, 2017
@dean0x7d dean0x7d deleted the detail-headers branch August 17, 2017 12:08
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