This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add header guard style to suggest use of #pragma once
Needs ReviewPublic

Authored by KyleFromKitware on Jan 19 2023, 8:05 AM.

Details

Reviewers
njames93
Group Reviewers
Restricted Project
Summary

Co-Authored-by: Igor-Mikhail-Valentin Glebov <iglebov@albany.edu>

Diff Detail

Event Timeline

KyleFromKitware created this object with edit policy "KyleFromKitware (Kyle Edwards)".
Herald added a project: Restricted Project. · View Herald Transcript
KyleFromKitware requested review of this revision.Jan 19 2023, 8:05 AM

Added documentation.

Can you explain a bit about why you want to promote use of #pragma once? It's not been standardized specifically because of how many known issues it causes, so it strikes me as odd to claim it's a modernization of a code base to use it. To me, I'd almost rather have the opposite behavior -- convert use of #pragma once into working header guards.

We have this check in a custom clang-tidy module that runs on CMake's CI system. Other people have also sought out such a check, and I figured it made sense to upstream it instead of keeping it stuck in our CI. Even though it's not standard, other people might still want to use it. Perhaps there's a better category for it than modernize?

Given the fact its non-standard, it has caveats and peoples eagerness to blindly enable all checks (or all checks from a module).
I feel this check would likely cause more harm than good.

and peoples eagerness to blindly enable all checks (or all checks from a module)

Perhaps there's some way we can have it disabled by default and not enabled unless they explicitly enable it (similar to -Wall vs -Wextra/-Weverything)?

I agree that it should not be standard, but it works in the vast majority of use cases. Drake is another very large C++ project that uses #pragma once everywhere. The demand for #pragma once exists, as does the demand for a check to enforce it. If we don't land this in LLVM proper, where would be a better place for it?

lebedev.ri added a subscriber: lebedev.ri.EditedJan 19 2023, 10:24 AM

Given the fact its non-standard, it has caveats and peoples eagerness to blindly enable all checks (or all checks from a module).
I feel this check would likely cause more harm than good.

Shall we remove abseil checks?
Shall we remove libc++-specific checks?
Shall we remove webkit checks?
Shall we remove linuxkernel checks?
Shall we remove backwards-compatibility checks?

Nothing is ever useful for everyone. Much like -Weverything,
enabling all checks comes with an explcit caveat that
one needs to disable the checks that are not applicable for the codebase.

+1 to having this check.

We have this check in a custom clang-tidy module that runs on CMake's CI system. Other people have also sought out such a check, and I figured it made sense to upstream it instead of keeping it stuck in our CI. Even though it's not standard, other people might still want to use it. Perhaps there's a better category for it than modernize?

Okay, so it's basically that folks want a check for header guards but want to control the style of guard. I think a generic check for header guards is valuable (where the user can configure which style to prefer but we probably default to the standard include guards), but a check for one specific form is less helpful IMO. Once we have a generic check, we could make the llvm header guard check into an alias of the generic check.

In terms of what module, I think a generic check would make sense in readability because this isn't about correctness so much as style.

Given the fact its non-standard, it has caveats and peoples eagerness to blindly enable all checks (or all checks from a module).
I feel this check would likely cause more harm than good.

Shall we remove abseil checks?
Shall we remove libc++-specific checks?
Shall we remove webkit checks?
Shall we remove backwards-compatibility checks?

Nothing is ever useful for everyone. Much like -Weverything,
enabling all checks comes with an explcit caveat that
one needs to disable the checks that are not applicable for the codebase.

+1 to having this check.

We're not asking for it to be useful to everyone; we are asking for justification for the current proposed form because there are problems with what's proposed. We're trying to figure out what the correct approach is (fwiw, I'd be opposed to what's proposed as-is but would be fine with a tweaked proposal).

KyleFromKitware added a comment.EditedJan 19 2023, 10:30 AM

We're trying to figure out what the correct approach is (fwiw, I'd be opposed to what's proposed as-is but would be fine with a tweaked proposal).

Thanks for clarifying this. I am absolutely open to tweaking this proposal (moving it to a different module, disabling it by default, etc.) My biggest concern is to upstream this functionality so that others can use it. I stuck it in modernize but would absolutely be happy to move it somewhere else.

In terms of a "generic check", there is already the HeaderGuardCheck, but that has to be extended to be specific to the project's naming convention. And I don't think there can be a generic check that converts #pragma once to a header guard - when it adds the header guard, what does it name it? It has to know the project's naming convention. If we want a check that replaces #pragma once with a header guard, our best bet would be modifying HeaderGuardCheck to do this, since it already does most of the needed work. However, such functionality is orthogonal to my proposal here.

In terms of a "generic check", there is already the HeaderGuardCheck, but that has to be extended to be specific to the project's naming convention. And I don't think there can be a generic check that converts #pragma once to a header guard - when it adds the header guard, what does it name it? It has to know the project's naming convention.

It doesn't have to -- for example, we can name the guard based on the path to the header. e.g., foo/include/bar/baz.h could use FOO_INCLUDE_BAR_BAZ_H as the header guard.

If we want a check that replaces #pragma once with a header guard, our best bet would be modifying HeaderGuardCheck to do this, since it already does most of the needed work. However, such functionality is orthogonal to my proposal here.

It's orthogonal except for the design concerns. Basically, I don't think we should have one check for "go to header guards" and another check for "go to pragma once" -- IMO we should have one check to do either approach. @njames93 is correct that a common use case for clang-tidy is "enable all checks" so having two checks that do opposite things gets awkward in practice.

It doesn't have to -- for example, we can name the guard based on the path to the header. e.g., foo/include/bar/baz.h could use FOO_INCLUDE_BAR_BAZ_H as the header guard.

But then this would conflict with LLVMHeaderGuardCheck - the LLVM check would suggest one thing and the generic check would suggest something different.

It's orthogonal except for the design concerns. Basically, I don't think we should have one check for "go to header guards" and another check for "go to pragma once" -- IMO we should have one check to do either approach. @njames93 is correct that a common use case for clang-tidy is "enable all checks" so having two checks that do opposite things gets awkward in practice.

See above - a generic check would also cause a conflict if they blindly enabled all checks. I think that no matter what we do here, enabling all checks is going to cause a conflict of some kind. The only way to avoid this is if we overhaul the existing HeaderGuardCheck and make its naming convention configurable by a configuration option as opposed to having to extend the C++ class to calculate the name.

The only way to avoid this is if we overhaul the existing HeaderGuardCheck and make its naming convention configurable by a configuration option as opposed to having to extend the C++ class to calculate the name.

Actually, this might be realistic. We could add a new registry for header guard styles. So then instead of having separate checks named llvm-header-guard and such, we have a single check named header-guard which can then select between different header guard styles (llvm, pragma-once, etc.) Projects could even create their own custom guard styles and register them in this new registry.

Given the fact its non-standard, it has caveats and peoples eagerness to blindly enable all checks (or all checks from a module).
I feel this check would likely cause more harm than good.

Shall we remove abseil checks?
Shall we remove libc++-specific checks?
Shall we remove webkit checks?
Shall we remove backwards-compatibility checks?

Nothing is ever useful for everyone. Much like -Weverything,
enabling all checks comes with an explcit caveat that
one needs to disable the checks that are not applicable for the codebase.

+1 to having this check.

We're not asking for it to be useful to everyone; we are asking for justification for the current proposed form because there are problems with what's proposed. We're trying to figure out what the correct approach is (fwiw, I'd be opposed to what's proposed as-is but would be fine with a tweaked proposal).

Thanks!

Basically, anything that extends HeaderGuardCheck (or HeaderGuardBase from D142121) is really just a fancy configuration option, rather than a separate check, and should be treated as such. My proposal is this:

  1. Create a new base class called HeaderGuardStyle which controls all behavior that happens after a header guard or #pragma once is discovered, and create associated llvm::Registry
  2. Modify HeaderGuardCheck to select a HeaderGuardStyle from the registry based on a configuration option
  3. Create a class called MacroHeaderGuardStyle which extends HeaderGuardStyle and basically does what D142121 does now
  4. Rename LLVMHeaderGuardCheck to LLVMHeaderGuardStyle and modify it to extend MacroHeaderGuardStyle and add it to the registry
  5. Add PragmaOnceHeaderGuardStyle which extends HeaderGuardStyle and add it to the registry

The refactoring that I've done in D142121 is most of the way to being able to implement my above proposal. I just need to shift a few things around and add the new classes. @aaron.ballman if this proposal is good with you I'll get started on it.

The refactoring that I've done in D142121 is most of the way to being able to implement my above proposal. I just need to shift a few things around and add the new classes. @aaron.ballman if this proposal is good with you I'll get started on it.

I think this sounds like a sensible approach to me, but I'd like to make sure @njames93 agrees before you start doing any work on it.

Can you just elaborate on what the registry is used for? Is the plan to support potentially dynamically loading HeaderGuardStyle classes.
If thats the case, Then I'd argue we don't need the HeaderGuardBase check class, We can just create a HeaderGuardCheck class that creates an instance of the HeaderGuardStyle based on the configuration options.
Then to maintain backwards compatibility the llvm-header-guard could be an alias with the default style option set to llvm for example.

Also this goes for all your PRs, can you please upload them in future with full context git diff -U999999 should do it. Makes revewing changes easier and removes those Context not available message on phab

Can you just elaborate on what the registry is used for? Is the plan to support potentially dynamically loading HeaderGuardStyle classes.

Yes.

If thats the case, Then I'd argue we don't need the HeaderGuardBase check class, We can just create a HeaderGuardCheck class that creates an instance of the HeaderGuardStyle based on the configuration options.

Yes, that was my plan, sorry for not making that explicit. HeaderGuardStyle is supposed to be analogous to HeaderGuardBase and MacroHeaderGuardStyle is supposed to be analogous to HeaderGuardCheck from D142121.

Then to maintain backwards compatibility the llvm-header-guard could be an alias with the default style option set to llvm for example.

Sounds good. What should the default for regular header-guard be then?

Also this goes for all your PRs, can you please upload them in future with full context git diff -U999999 should do it. Makes revewing changes easier and removes those Context not available message on phab

Will do.

Please note, HeaderFileExtensions is becoming a global option in this patch to avoid duplication, which will land in the next 2 days.
https://reviews.llvm.org/D141000

Therefore you'll need to update the patch based on that.

KyleFromKitware retitled this revision from [clang-tidy] Add check to suggest use of #pragma once to [clang-tidy] Add header guard style to suggest use of #pragma once.

Refactored to use HeaderGuardStyle.

KyleFromKitware changed the edit policy from "KyleFromKitware (Kyle Edwards)" to "All Users".Jan 27 2023, 8:52 AM

@aaron.ballman @njames93 could you guys please take another look at this?

aaron.ballman added inline comments.Feb 3 2023, 6:48 AM
clang-tools-extra/docs/clang-tidy/checks/readability/header-guard.rst
31

It's also not in the C standard, so reworded slightly.

clang-tools-extra/unittests/clang-tidy/ReadabilityModuleTest.cpp
569–571

How about a test for:

#if !defined(HEADER_GUARD_H)
#define HEADER_GUARD_H 1

#endif

and a test for what happens when there's no header guard or pragma once in the file.

Updated with review feedback.

KyleFromKitware added inline comments.Feb 3 2023, 8:03 AM
clang-tools-extra/unittests/clang-tidy/ReadabilityModuleTest.cpp
569–571

How about a test for:

#if !defined(HEADER_GUARD_H)
#define HEADER_GUARD_H 1

#endif

Good catch. The case of !defined() was not being handled by the existing header guard check except to basically ignore it. The #pragma once check crashed on this case. I've updated it to also ignore the !defined() case.

and a test for what happens when there's no header guard or pragma once in the file.

I already have this, see the "neither" test at the bottom.

Updated documentation.

Removed comma from documentation.

KyleFromKitware marked an inline comment as done.Feb 3 2023, 8:05 AM
KyleFromKitware marked an inline comment as done.

LGTM assuming precommit CI doesn't discover any surprises, but hopefully @njames93 or @carlosgalvezp can weigh in as well since they've been in this code more recently than I have.

clang-tools-extra/unittests/clang-tidy/ReadabilityModuleTest.cpp
569–571

and a test for what happens when there's no header guard or pragma once in the file.

I already have this, see the "neither" test at the bottom.

Ah! I was misreading:

EXPECT_EQ("#pragma once\n"
          "void neither();\n",
          runPragmaOnceCheck("void neither();\n", "neither.h",
                             StringRef("use #pragma once")));

as having the pragma once in there, which is backwards. Thanks!

CI on all seven commits in this stack looks good.