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

include: remove config.h from headers #752

Closed
wants to merge 1 commit into from

Conversation

rauteric
Copy link
Contributor

Header files shouldn't include config.h

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Header files shouldn't include `config.h`

Signed-off-by: Eric Raut <[email protected]>
@rauteric rauteric requested a review from a team as a code owner December 12, 2024 01:51
Copy link
Contributor

@bwbarrett bwbarrett left a comment

Choose a reason for hiding this comment

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

We're probably solving the wrong problem here, but this also isn't wrong, so approved.

@aws-nslick
Copy link
Contributor

aws-nslick commented Dec 12, 2024

I'm somewhat against this. Some of these are just unnecessary, but some are not, eg: HAVE_DECL_FI_MR_DMABUF must be defined for nccl_ofi_mr.h to do the right thing.

I don't see any good reason why it should be possible to include nccl_ofi_mr.h without that being defined. We've been bit by this before.

The cleanest way to do this IMO is just to get rid of config.h entirely by removing AC_CONFIG_FILE(config.h) (or whatever) from configure.ac, so that -DHAVE_DECL_FI_MR_DMABUF=1 is passed on every invocation of the compiler and you never have to worry about config.h being the first include or making sure that every file is including it in the first place. I've proposed that before but it wasn't popular.

So as long as we have config.h, and it needs to be included by someone (either the TU itself, where every header relies on a transitive previous inclusion of config.h, or by the header itself pulling in config.h when it needs some definition from config.h), I think the latter is preferable and the file that uses it should be responsible for including it -- mostly based on arguments from Why Include What You Use? -- it's really nice to be able to grep for a header and see at a glance how files connect to each other.

There's an argument to be made that project public headers should be stateless and not have any configuration, but that's not the state of the project and removing the include from the header doesn't change that, it just means that there's a transitive* inclusion of config.h that must occur instead.

@rauteric
Copy link
Contributor Author

rauteric commented Dec 12, 2024

Nick convinced me that this is not sufficient to deal with our benchmark code properly, so I will come up with something else that doesn't require this patch.

I think right now we're a bit inconsistent with whether we include config.h in the headers or not, but I'm fine leaving well enough alone.

@rauteric rauteric closed this Dec 12, 2024
@aws-nslick
Copy link
Contributor

aws-nslick commented Dec 13, 2024

A portion of this commit was fine with me (just in terms of removing totally unnecessary #includes, ie: the one in the tuner).

If there's buy in from both of you, I'd like to just get iwyu running in github actions on every commit similar to other compile-time checks we have. There's a million ways to solve header consistency, but at least this would be automatic and consistent, and we wouldn't ever have to think about it again.

I'd actually proposed this previously in #351 (0e2d51d and f34a7ae)

@bwbarrett
Copy link
Contributor

Unless we need a ton of workaround pragmas, I'm fine adding IWYU in the list of compile time checkers we run on every PR. I did try it with the version included in U20 without success, but didn't put a lot of time into it.

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.

3 participants