pybind / pybind/pybind11

Header reorganization

Open
#708 6 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C++
Stars
18k
Forks
2.3k
Avg merge
5d 17h
Merged PRs (30d)
10

Description

#679 initially added a detail/ directory to the header includes. It was withdrawn to be discussed later in a separate issue/PR; this is meant to be the start of discussion. Also read the following not as a done deal, but as a request for comments where I'm mostly thinking out loud to start the discussion.

I think we should make the move for most (or even all?) of our detail namespace code. Basically, all the things in class_support.h are an obvious first start to move to detail/class.h; but there are also many other potential additions:

Detail headers

  • the various template meta-code in common.h (for things like satisfied_all_of, would fit nicely into a detail/meta.h. It's relatively centralized now (in common.h), but that hasn't always been the case and has led to me, at least, occassionally duplicating meta template functionality that was already implemented (but implemented elsewhere, i.e. immediately before it was used/needed).

  • The same sort of moving of detail namespace bits into detail-namespace companions would be a nice cleanup to the non-detail code. E.g. pybind11.h's generic_type and init classes, keep alive implementation, etc. are all already in the detail namespace; moving them into a detail directory header would be appropriate.

Type casters

Along a similar line, we could move type caster implementations into their own namespace, pybind11::cast, and directory, pybind11/cast. For now, for backwards compatibility, cast would be a namespace alias of detail, but the plan would be to eventually drop the alias so that all type casters live as pybind11::cast::type_caster<T>, thus having external type casters put themselves in the detail namespace.

Thus you would include pybind11/cast/stl.h for stl casters, pybdin11/cast/eigen.h for eigen casters, etc. (The current pybind11/stl.h, pybind11/eigen.h would just one-line compatibility headers that just include the new location).

Many of the built-in casters would be usefully moved as well:

  • the arithmetic caster in cast/arithmetic.h
  • std::string and char* casters to cast/string.h
  • std::pair/std::tuple into cast/tuple.h
  • pointer casts, null casters, generic casters I think could remain in cast.h.
  • the argument loader seems a bit out of place in cast.h—I think it's there mainly for historic reasons, since it was originally part of the std::tuple caster. Perhaps detail/argloader.h?

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reviewing class_support.h, common.h, pybind11.h, cast.h, stl.h, and eigen.h to understand the current header and namespace layout. The issue is an open-ended discussion, so the work is not yet defined; done would require an agreed organization and compatibility plan before implementation.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, python
Domain
devtools
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
20/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.