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

New header convention #2572

Draft
wants to merge 4 commits into
base: devel
Choose a base branch
from

Conversation

jorisv
Copy link
Contributor

@jorisv jorisv commented Feb 11, 2025

This PR is a POC about include convention.

Right now, there is no include convention. This lead to some issues:

  • Code discrepancy
  • Cyclic include
  • LSP is not working well (since some includes are missing)
  • Big dependency chain (since they are probable unused include in most file)

Since Pinocchio is header only, we can't use the include what you use convention since it will create a lot of cycle.

In this PR we propose to change the include convention as the following:

  • Public headers: xxx.hpp
  • Private headers:
    • Definition: xxx-def.hxx
    • Declaration or declaration and definition when they are not splitted: xxx.hxx
    • Explicit template instantiation: xxx-inst.hxx

With the following rules:

  • Private header can't include anything
  • Public header can include private header

This should prevent cyclic include and with some .clangd trick LSP processing will be improved.

All is described in development/convention.md.

Constraints

  • Public API should be preserved
  • Authors should be preserved

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

👋 Hi,
This is a reminder message to assign an extra build label to this Pull Request if needed.
By default, this PR will be build with minimal build options (URDF support and Python bindings)
The possible extra labels are:

  • build_collision (build Pinocchio with coal support)
  • build_casadi (build Pinocchio with CasADi support)
  • build_autodiff (build Pinocchio with CppAD support)
  • build_codegen (build Pinocchio with CppADCodeGen support)
  • build_extra (build Pinocchio with extra algorithms)
  • build_mpfr (build Pinocchio with Boost.Multiprecision support)
  • build_sdf (build Pinocchio with SDF parser)
  • build_accelerate (build Pinocchio with APPLE Accelerate framework support)
  • build_all (build Pinocchio with ALL the options stated above)

Thanks.
The Pinocchio development team.

@ManifoldFR ManifoldFR self-assigned this Mar 5, 2025
@ManifoldFR
Copy link
Member

ManifoldFR commented Mar 5, 2025

Here's some feedback about the proposal.

  • The example with a.hpp and b.hpp seems a bit contrived since the cycle can be broken by forward-declaration of either struct A or struct B in either file because of the pointers; maybe change something or add something extra to one of the headers that means inclusion is required for the code to compile. I'd suggest doing struct A { B b; } since by-value data members require definition of the member's type.
  • I like the idea of having explicit rules for the file extensions of the public headers and that they can include private headers.
  • "Private headers can't include anything" might be a bit stringent, no? Including utility, macro, or math header files should be okay IMO
  • Why rename the template instantiation declarations to xxx-tpl.hxx? I thought xxx.txx was good and explicit enough - we just need to ensure the formatters, git hooks etc properly account for them.
  • Not sure about xxx-def.hxx/xxx-decl.hxx. Maybe just plain xxx.hxx for the declaration and xxx-impl.hxx for the definitions/implementations?

@jorisv
Copy link
Contributor Author

jorisv commented Mar 6, 2025

* The example with `a.hpp` and `b.hpp` seems a bit contrived since the cycle can be broken by forward-declaration of either `struct A` or `struct B` in either file because of the pointers; maybe change something or add something extra to one of the headers that means inclusion is _required_ for the code to compile. I'd suggest doing `struct A { B b; }` since by-value data members require definition of the member's type.

Sure this example is simple. What I want to show is not the forward declaration issue, more the include cycle.
You include a file, hopping to have his content, but since you're already included by it, you create a cycle, then the include directive will do nothing. The right way to show how bad is it would be to have a really long include chain, but it's really boring to setup.

* "Private headers can't include anything" might be a bit stringent, no? Including utility, macro, or math header files should be okay IMO

If you allow some includes, how do you prevent include cycle ?
The only include that can be allowed is headers from external libraries (I think they allow it in Eigen).
We can maybe allow that if in the future, but first, I prefer to try the hard way.

* Why rename the template instantiation declarations to `xxx-tpl.hxx`? I thought `xxx.txx` was good and explicit enough - we just need to ensure the formatters, git hooks etc properly account for them.

I rename it into xxx-expl.hxx because xxx-tlp is already taken (se3-tpl.hpp).
I don't see the point to have a new extension. It's only add us more work to manage this custom extension type. I don't see any good point keep it.

* Not sure about `xxx-def.hxx`/`xxx-decl.hxx`. Maybe just plain `xxx.hxx` for the declaration and `xxx-impl.hxx` for the definitions/implementations?

-def and -decl are more easily searchable I think. Also, I like to be explicit. But you're right on headers without split definition and declaration, this can be confusing.

@jorisv
Copy link
Contributor Author

jorisv commented Mar 6, 2025

@ManifoldFR I have changed xxx-expl.hxx to xxx-inst.hxx in the PR description. Also, xxx-decl.hxx have been changed to xxx.hxx.

I will change the convention.md and the migration script later.

jorisv added 4 commits March 6, 2025 15:15
dev: First working version of the refactor script

dev: Raise a nice error when we can't find guards in refactor_headers.py

Apply new convention to refactor_headers.py

dev: Apply new commit convention
@jorisv jorisv force-pushed the topic/refactor-headers branch from 6528383 to 4c68d20 Compare March 6, 2025 14:17
@jorisv jorisv added the pr status wip To not review in weekly meeting label Mar 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr status wip To not review in weekly meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants