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

[KyleFromKitware]

Summary

Co-Authored-by: Igor-Mikhail-Valentin Glebov <[e-mail]>

[KyleFromKitware]

Added documentation.

[aaron.ballman]

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.

[KyleFromKitware]

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?

[njames93]

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.

[KyleFromKitware]

In D142123#4066351, @njames93 wrote:

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]

In D142123#4066351, @njames93 wrote:

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.

[aaron.ballman]

In D142123#4066023, @KyleFromKitware wrote:

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.

[aaron.ballman]

In D142123#4066447, @lebedev.ri wrote:

In D142123#4066351, @njames93 wrote:

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]

In D142123#4066460, @aaron.ballman wrote:

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.

[KyleFromKitware]

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.

[aaron.ballman]

In D142123#4066564, @KyleFromKitware wrote:

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.

[KyleFromKitware]

In D142123#4066676, @aaron.ballman wrote:

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.

[KyleFromKitware]

In D142123#4066706, @KyleFromKitware wrote:

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.

[lebedev.ri]

In D142123#4066460, @aaron.ballman wrote:

In D142123#4066447, @lebedev.ri wrote:

In D142123#4066351, @njames93 wrote:

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!

[KyleFromKitware]

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.

[aaron.ballman]

In D142123#4066805, @KyleFromKitware wrote:

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.

[njames93]

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.

[njames93]

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

[KyleFromKitware]

In D142123#4071099, @njames93 wrote:

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.

[carlosgalvezp]

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]

Refactored to use HeaderGuardStyle.

[KyleFromKitware]

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

[aaron.ballman]

clang-tools-extra/docs/clang-tidy/checks/readability/header-guard.rst

31 ↗

(On Diff #492634)

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

clang-tools-extra/unittests/clang-tidy/ReadabilityModuleTest.cpp

569–571 ↗

(On Diff #492634)

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.

[KyleFromKitware]

Updated with review feedback.

[KyleFromKitware]

clang-tools-extra/unittests/clang-tidy/ReadabilityModuleTest.cpp

569–571 ↗

(On Diff #492634)

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.

[KyleFromKitware]

Updated documentation.

[KyleFromKitware]

Removed comma from documentation.

[aaron.ballman]

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 ↗

(On Diff #492634)

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!

[KyleFromKitware]

CI on all seven commits in this stack looks good.

clang-tools-extra/clang-tidy/modernize/CMakeLists.txt

   UseNoexceptCheck.cpp
   UseNullptrCheck.cpp
   UseOverrideCheck.cpp
+  UsePragmaOnceCheck.cpp
   UseTrailingReturnTypeCheck.cpp
   UseTransparentFunctorsCheck.cpp
   UseUncaughtExceptionsCheck.cpp

clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp

 #include "UseNoexceptCheck.h"
 #include "UseNullptrCheck.h"
 #include "UseOverrideCheck.h"
+#include "UsePragmaOnceCheck.h"
 #include "UseTrailingReturnTypeCheck.h"
 #include "UseTransparentFunctorsCheck.h"
 #include "UseUncaughtExceptionsCheck.h"

     CheckFactories.registerCheck<UseNoexceptCheck>("modernize-use-noexcept");
     CheckFactories.registerCheck<UseNullptrCheck>("modernize-use-nullptr");
     CheckFactories.registerCheck<UseOverrideCheck>("modernize-use-override");
+    CheckFactories.registerCheck<UsePragmaOnceCheck>(
+        "modernize-use-pragma-once");
     CheckFactories.registerCheck<UseTrailingReturnTypeCheck>(
         "modernize-use-trailing-return-type");
     CheckFactories.registerCheck<UseTransparentFunctorsCheck>(

clang-tools-extra/clang-tidy/modernize/UsePragmaOnceCheck.h

+//===--- UsePragmaOnceCheck.h - clang-tidy ----------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEPRAGMAONCECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEPRAGMAONCECHECK_H
+
+#include "../utils/HeaderGuardBase.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// Check that suggests the use of #pragma once.
+/// The check supports these options:
+///   - `HeaderFileExtensions`: a semicolon-separated list of filename
+///     extensions of header files (The filename extension should not contain
+///     "." prefix). ";h;hh;hpp;hxx" by default.
+///
+///     For extension-less header files, using an empty string or leaving an
+///     empty string between ";" if there are other filename extensions.
+class UsePragmaOnceCheck : public utils::HeaderGuardBase {
+public:
+  UsePragmaOnceCheck(StringRef Name, ClangTidyContext *Context)
+      : HeaderGuardBase(Name, Context) {}
+
+  void onHeaderGuard(Preprocessor *PP, StringRef FileName, const FileEntry *FE,
+                     SourceLocation IfndefHash, SourceLocation Ifndef,
+                     SourceLocation IfndefToken, SourceLocation DefineHash,
+                     const Token &Define, SourceLocation EndIfHash,
+                     SourceLocation EndIf) override;
+
+  void onGuardlessHeader(
+      Preprocessor *PP, StringRef FileName, const FileEntry *FE,
+      SourceLocation StartLoc,
+      const std::vector<std::tuple<SourceLocation, Token, const MacroInfo *>>
+          &Macros) override;
+};
+} // namespace modernize
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEPRAGMAONCECHECK_H

clang-tools-extra/clang-tidy/modernize/UsePragmaOnceCheck.cpp

+//===--- UsePragmaOnceCheck.cpp - clang-tidy --------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "UsePragmaOnceCheck.h"
+#include "clang/Lex/Preprocessor.h"
+
+namespace clang::tidy::modernize {
+namespace {
+CharSourceRange goToLineEnd(Preprocessor *PP, SourceLocation StartLoc,
+                            SourceLocation EndLoc) {
+  SourceManager &SM = PP->getSourceManager();
+  FileID FID = SM.getFileID(EndLoc);
+  StringRef BufData = SM.getBufferData(FID);
+  const char *EndData = BufData.begin() + SM.getFileOffset(EndLoc);
+  Lexer Lex(EndLoc, PP->getLangOpts(), BufData.begin(), EndData, BufData.end());
+  // FIXME: this is a bit hacky to get ReadToEndOfLine work.
+  Lex.setParsingPreprocessorDirective(true);
+  Lex.ReadToEndOfLine();
+  return CharSourceRange::getCharRange(
+      StartLoc, SM.getLocForStartOfFile(FID).getLocWithOffset(
+                    Lex.getCurrentBufferOffset()));
+}
+} // namespace
+
+void UsePragmaOnceCheck::onHeaderGuard(
+    Preprocessor *PP, StringRef FileName, const FileEntry *FE,
+    SourceLocation IfndefHash, SourceLocation Ifndef,
+    SourceLocation IfndefToken, SourceLocation DefineHash, const Token &Define,
+    SourceLocation EndIfHash, SourceLocation EndIf) {
+  std::vector<FixItHint> FixIts;
+
+  HeaderSearch &HeaderInfo = PP->getHeaderSearchInfo();
+
+  HeaderFileInfo &Info = HeaderInfo.getFileInfo(FE);
+
+  CharSourceRange IfndefSrcRange = goToLineEnd(PP, IfndefHash, IfndefToken);
+  CharSourceRange DefineSrcRange =
+      goToLineEnd(PP, DefineHash, Define.getLocation());
+  CharSourceRange EndifSrcRange = goToLineEnd(PP, EndIfHash, EndIf);
+
+  if (Info.isPragmaOnce)
+    FixIts.push_back(FixItHint::CreateRemoval(IfndefSrcRange));
+  else
+    FixIts.push_back(
+        FixItHint::CreateReplacement(IfndefSrcRange, "#pragma once\n"));
+
+  FixIts.push_back(FixItHint::CreateRemoval(DefineSrcRange));
+  FixIts.push_back(FixItHint::CreateRemoval(EndifSrcRange));
+
+  diag(IfndefSrcRange.getBegin(), "use #pragma once") << FixIts;
+}
+
+void UsePragmaOnceCheck::onGuardlessHeader(
+    Preprocessor *PP, StringRef FileName, const FileEntry *FE,
+    SourceLocation StartLoc,
+    const std::vector<std::tuple<SourceLocation, Token, const MacroInfo *>>
+        &Macros) {
+  HeaderSearch &HeaderInfo = PP->getHeaderSearchInfo();
+
+  HeaderFileInfo &Info = HeaderInfo.getFileInfo(FE);
+  if (Info.isPragmaOnce)
+    return;
+
+  diag(StartLoc, "use #pragma once")
+      << FixItHint::CreateInsertion(StartLoc, "#pragma once\n");
+}
+} // namespace clang::tidy::modernize

clang-tools-extra/docs/clang-tidy/checks/list.rst

    `modernize-use-noexcept <modernize/use-noexcept.html>`_, "Yes"
    `modernize-use-nullptr <modernize/use-nullptr.html>`_, "Yes"
    `modernize-use-override <modernize/use-override.html>`_, "Yes"
+   `modernize-use-pragma-once <modernize/use-pragma-once.html>`_, "Yes"
    `modernize-use-trailing-return-type <modernize/use-trailing-return-type.html>`_, "Yes"
    `modernize-use-transparent-functors <modernize/use-transparent-functors.html>`_, "Yes"
    `modernize-use-uncaught-exceptions <modernize/use-uncaught-exceptions.html>`_, "Yes"

clang-tools-extra/docs/clang-tidy/checks/modernize/use-pragma-once.rst

+.. title:: clang-tidy - modernize-use-pragma-once
+
+modernize-use-pragma-once
+=========================
+
+Adds ``#pragma once`` (a widely-supported compiler extension) to header files
+and removes header guards if they are used (``#ifndef`` / ``#define`` /
+``#endif``).
+
+Note that ``#pragma once`` cannot be correct in all possible use cases and has
+not been added to the C++ standard. Not all projects will wish to use it.
+
+Options
+-------
+
+.. option:: HeaderFileExtensions
+
+   A comma-separated list of filename extensions of header files (the filename
+   extensions should not include "." prefix). Default is "h,hh,hpp,hxx".
+   For header files without an extension, use an empty string (if there are no
+   other desired extensions) or leave an empty element in the list. E.g.,
+   "h,hh,hpp,hxx," (note the trailing comma).

clang-tools-extra/unittests/clang-tidy/ModernizeModuleTest.cpp

 //===----------------------------------------------------------------------===//
 #include "ClangTidyTest.h"
 #include "modernize/IntegralLiteralExpressionMatcher.h"
+#include "modernize/UsePragmaOnceCheck.h"
 #include "clang/Lex/Lexer.h"
 #include "gtest/gtest.h"
 

 INSTANTIATE_TEST_SUITE_P(IntegralLiteralExpressionMatcherTests, SizeTest,
                          ::testing::ValuesIn(SizeParams));
 
+namespace {
+std::string runPragmaOnceCheck(StringRef Code, const Twine &Filename,
+                               std::optional<StringRef> ExpectedWarning,
+                               std::map<StringRef, StringRef> PathsToContent =
+                                   std::map<StringRef, StringRef>()) {
+  std::vector<ClangTidyError> Errors;
+  std::string Result = test::runCheckOnCode<modernize::UsePragmaOnceCheck>(
+      Code, &Errors, Filename, std::string("-xc++-header"), ClangTidyOptions{},
+      std::move(PathsToContent));
+  if (Errors.size() != (size_t)ExpectedWarning.has_value())
+    return "invalid error count";
+  if (ExpectedWarning && *ExpectedWarning != Errors.back().Message.Message)
+    return "expected: '" + ExpectedWarning->str() + "', saw: '" +
+           Errors.back().Message.Message + "'";
+  return Result;
+}
+} // namespace
+
+TEST(UsePragmaOnceCheckTest, AddPragmaOnce) {
+  EXPECT_EQ("#pragma once\n"
+            "\n"
+            "void headerGuard();\n"
+            "\n",
+            runPragmaOnceCheck("#ifndef HEADER_GUARD_H\n"
+                               "#define HEADER_GUARD_H\n"
+                               "\n"
+                               "void headerGuard();\n"
+                               "\n"
+                               "#endif // HEADER_GUARD_H\n",
+                               "header-guard.h",
+                               StringRef("use #pragma once")));
+  EXPECT_EQ("#pragma once\n"
+            "\n"
+            "void pragmaOnce();\n",
+            runPragmaOnceCheck("#pragma once\n"
+                               "\n"
+                               "void pragmaOnce();\n",
+                               "pragma-once.h", std::nullopt));
+  EXPECT_EQ("#pragma once\n"
+            "\n"
+            "void both();\n"
+            "\n",
+            runPragmaOnceCheck("#ifndef BOTH_H\n"
+                               "#define BOTH_H\n"
+                               "#pragma once\n"
+                               "\n"
+                               "void both();\n"
+                               "\n"
+                               "#endif // BOTH_H\n",
+                               "both.h", StringRef("use #pragma once")));
+  EXPECT_EQ("#pragma once\n"
+            "void neither();\n",
+            runPragmaOnceCheck("void neither();\n", "neither.h",
+                               StringRef("use #pragma once")));
+}
+
 } // namespace test
 } // namespace tidy
 } // namespace clang