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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unnecessary includes from clap.h #388

Draft
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

Trinitou
Copy link
Contributor

let's see if this passes the pull-request workflows... 馃槈

I think this removes some clutter. here is my theory:

  • plugin.h is included via factory/plugin-factory.h and plugin extensions (makes sense to me)
    • host.h and plugin-features.h are included via plugin.h (makes sense as well)
  • universal-plugin-id.h is included via factory/preset-discovery.h (where it is actually used)

@Trinitou Trinitou mentioned this pull request Jan 18, 2024
@abique
Copy link
Contributor

abique commented Jan 18, 2024

I'm not sure about that one.

The goal of clap.h is to pull everything except the draft.
If x.h is included by y.h and we decide to not add x.h to clap.h, you still get x.h indirectly by including clap.h.
But, if one day x.h isn't included anymore from y.h, then we need to remember to add back x.h to clap.h.

If we directly include all headers in clap.h, we don't rely upon indirect inclusion.

We use #pragma once so I believe that it is a piece of cake for the compiler and the overhead is negligible.

Having everything in clap.h facilitate navigation within an IDE maybe 馃 .

@baconpaul
Copy link
Collaborator

If clap.h is meant to be universal header I agree it should include things directly even if they are also included transitively

@Trinitou
Copy link
Contributor Author

Trinitou commented Jan 18, 2024

Should we close this (incomplete) pull-request as we want a more universal solution for all includes? Or should we leave it here and bring it into a 1.2.1 discussion later? + Maybe change it into a draft?

@baconpaul
Copy link
Collaborator

Let鈥檚 change it to draft so we don鈥檛 forget

@Trinitou Trinitou marked this pull request as draft January 19, 2024 06:20
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.

None yet

3 participants