[clang-tidy] Add a generic header guard checker + LLVM implementation.

[bkramer]

Summary

The implementation is split into a generic part and a LLVM-specific part.
Other codebases can implement it with their own style. The specific features
supported are:

• Verification (and fixing) of header guards against a style based on the file path
• Automatic insertion of header guards for headers that are missing them
• A warning when the header guard doesn't enable our fancy header guard optimization

(e.g. when there's an include preceeding the guard)

• Automatic insertion of a comment with the guard name after #endif.

For the LLVM style we disable #endif comments for now, they're not very common
in the codebase. We also only flag headers in the include directories, there
doesn't seem to be a common style outside.

[klimek]

clang-tidy/llvm/HeaderGuardCheck.cpp

17

Is it intentional that this finds lib/internal/Bincludes.h?

clang-tidy/llvm/HeaderGuardCheck.h

22

You might want to run the override clang-tidy checker :P

clang-tidy/utils/HeaderGuard.cpp

21–22

Perhaps add a FIXME to put this somewhere more central.

30

Do we want an error for something like cleanPath("../../../../some/path")?

72–73

s/Defines/defined/; s/Name/name/

104–127

Pull this into an extra method.

105–106

s/ifndef/Ifndef/

111–112

s/compute/compute, /

132–146

Pull this into an extra method.

149–190

Pull this into an extra method.

166

s/TODO/FIXME/

171

Any reason this an CPPVarUnder are not declared next to each other?

clang-tidy/utils/HeaderGuard.h

24

I think this looks much nicer in doxygen if it starts with:
"Returns true if..."
But I can see how you might want it differently as it's intended to be implemented by others. Still, I'd at least start sentences with a capital letter.

28–29

Can't parse the sentence.

unittests/clang-tidy/LLVMModuleTest.cpp

94–101

I somehow don't believe these are enough tests :)

[bkramer]

Addressed review comments.

[alexfh]

clang-tidy/utils/HeaderGuard.cpp

126

This class is stateful. AFAIU, it's reused when a tool is run with several translation units (clang-tidy x.cpp y.cpp z.cpp), so it should clean its state here.

[klimek]

I still doubt we have enough tests. Otherwise LG.

clang-tidy/utils/HeaderGuard.cpp

173

s/emits/Emits/

clang-tidy/utils/HeaderGuard.h

30–32

:gq

[bkramer]

• Clean PPCallbacks state after leaving file.
• Comment fixes.
• Also apply the style to header files outside of include/

[alexfh]

LG

clang-tidy/llvm/HeaderGuardCheck.cpp

22

Can we use the location of the compilation database to make project-relative paths? We can easily pass compilation database to the check via ClangTidyContext, if need be. We should consider this, but it shouldn't block this patch, though.

clang-tidy/llvm/HeaderGuardCheck.h

22

Why false? Aren't #endif comments used in LLVM?

25

Do you need to provide a default argument in the overridden method? Are you using this specific method with one argument?

[bkramer]

clang-tidy/llvm/HeaderGuardCheck.cpp

22

That's probably the right thing to do in the long term. Using absolute paths is a hack and might break if the user has a weird setup. We will get FileEntries with absolute paths from time to time (e.g. #include "./foo.h" creates them), so canonicalizing them with the compilation database seems like a nice way around this problem. It would break using without a database around (header files are often missing in the database), not sure what to do about that.

clang-tidy/llvm/HeaderGuardCheck.h

22

Some files do, the majority of headers does not, so I disabled them for now.

25

I am calling it, but it should only be there in the base class. This was copy and paste failing.

[bkramer]

Closed by commit rL215548 (authored by d0k).

clang-tidy/CMakeLists.txt

add_clang_library(clangTidy
   clangStaticAnalyzerFrontend
   clangTooling
   )
 
 add_subdirectory(tool)
 add_subdirectory(llvm)
 add_subdirectory(google)
 add_subdirectory(misc)
+add_subdirectory(utils)

clang-tidy/llvm/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyLLVMModule
+  HeaderGuardCheck.cpp
   IncludeOrderCheck.cpp
   LLVMTidyModule.cpp
   NamespaceCommentCheck.cpp
   TwineLocalCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
+  clangTidyUtils
   )

clang-tidy/llvm/HeaderGuardCheck.h

+//===--- HeaderGuardCheck.h - clang-tidy ------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_HEADER_GUARD_CHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_HEADER_GUARD_CHECK_H
+
+#include "../utils/HeaderGuard.h"
+
+namespace clang {
+namespace tidy {
+
+/// Finds and fixes header guards that do not adhere to LLVM style.
+class LLVMHeaderGuardCheck : public HeaderGuardCheck {
+public:
+  bool shouldSuggestEndifComment(StringRef Filename) override { return false; }
+  bool shouldFixHeaderGuard(StringRef Filename) override;
+  std::string getHeaderGuard(StringRef Filename,
+                             StringRef OldGuard = StringRef()) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_HEADER_GUARD_CHECK_H

clang-tidy/llvm/HeaderGuardCheck.cpp

+//===--- HeaderGuardCheck.cpp - clang-tidy --------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "HeaderGuardCheck.h"
+
+namespace clang {
+namespace tidy {
+
+bool LLVMHeaderGuardCheck::shouldFixHeaderGuard(StringRef Filename) {
+  return Filename.find("include/") != StringRef::npos;
+}
+
+std::string LLVMHeaderGuardCheck::getHeaderGuard(StringRef Filename,
+                                                 StringRef OldGuard) {
+  size_t PostInclude = Filename.find("include/") + std::strlen("include/");
+  std::string Guard = Filename.substr(PostInclude);
+
+  std::replace(Guard.begin(), Guard.end(), '/', '_');
+  std::replace(Guard.begin(), Guard.end(), '.', '_');
+  std::replace(Guard.begin(), Guard.end(), '-', '_');
+
+  // The prevalent style in clang is LLVM_CLANG_FOO_BAR_H
+  if (StringRef(Guard).startswith("clang"))
+    Guard = "LLVM_" + Guard;
+
+  return StringRef(Guard).upper();
+}
+
+} // namespace tidy
+} // namespace clang

clang-tidy/llvm/LLVMTidyModule.cpp

 //===--- LLVMTidyModule.cpp - clang-tidy ----------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "HeaderGuardCheck.h"
 #include "IncludeOrderCheck.h"
 #include "NamespaceCommentCheck.h"
 #include "TwineLocalCheck.h"
 
 namespace clang {
 namespace tidy {
 
 class LLVMModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.addCheckFactory(
+        "llvm-header-guard", new ClangTidyCheckFactory<LLVMHeaderGuardCheck>());
+    CheckFactories.addCheckFactory(
         "llvm-include-order", new ClangTidyCheckFactory<IncludeOrderCheck>());
     CheckFactories.addCheckFactory(
         "llvm-namespace-comment",
         new ClangTidyCheckFactory<NamespaceCommentCheck>());
     CheckFactories.addCheckFactory(
         "llvm-twine-local",
         new ClangTidyCheckFactory<TwineLocalCheck>());
   }

clang-tidy/utils/CMakeLists.txt

+set(LLVM_LINK_COMPONENTS support)
+
+add_clang_library(clangTidyUtils
+  HeaderGuard.cpp
+
+  LINK_LIBS
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangLex
+  clangTidy
+  )

clang-tidy/utils/HeaderGuard.h

+//===--- HeaderGuard.h - clang-tidy -----------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_HEADER_GUARD_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_HEADER_GUARD_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// \brief Finds and fixes header guards.
+class HeaderGuardCheck : public ClangTidyCheck {
+public:
+  void registerPPCallbacks(CompilerInstance &Compiler) override;
+
+  /// \brief Returns true if the checker should suggest inserting a trailing
+  /// comment on the #endif of the header guard. It will use the same name as
+  /// returned by getHeaderGuard.
+  virtual bool shouldSuggestEndifComment(StringRef Filename);
+  /// \brief Returns true if the checker should suggest changing an existing
+  /// header guard to the string returned by getHeaderGuard.
+  virtual bool shouldFixHeaderGuard(StringRef Filename);
+  /// \brief Returns true if the checker should add a header guard to the file
+  /// if
+  /// it has none.
+  virtual bool shouldSuggestToAddHeaderGuard(StringRef Filename);
+  /// \brief Get the canonical header guard for a file.
+  virtual std::string getHeaderGuard(StringRef Filename,
+                                     StringRef OldGuard = StringRef()) = 0;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_HEADER_GUARD_H

clang-tidy/utils/HeaderGuard.cpp

+//===--- HeaderGuard.cpp - clang-tidy -------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "HeaderGuard.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Lex/PPCallbacks.h"
+#include "clang/Lex/Preprocessor.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Support/Path.h"
+
+namespace clang {
+namespace tidy {
+
+/// \brief canonicalize a path by removing ./ and ../ components.
+// FIXME: Consider moving this to llvm::sys::path.
+static std::string cleanPath(StringRef Path) {
+  SmallString<256> NewPath;
+  for (auto I = llvm::sys::path::begin(Path), E = llvm::sys::path::end(Path);
+       I != E; ++I) {
+    if (*I == ".")
+      continue;
+    if (*I == "..") {
+      // Drop the last component.
+      NewPath.resize(llvm::sys::path::parent_path(NewPath).size());
+    } else {
+      if (!NewPath.empty())
+        NewPath += '/';
+      NewPath += *I;
+    }
+  }
+  return NewPath.str();
+}
+
+namespace {
+class HeaderGuardPPCallbacks : public PPCallbacks {
+public:
+  explicit HeaderGuardPPCallbacks(Preprocessor *PP, HeaderGuardCheck *Check)
+      : PP(PP), Check(Check) {}
+
+  void FileChanged(SourceLocation Loc, FileChangeReason Reason,
+                   SrcMgr::CharacteristicKind FileType,
+                   FileID PrevFID) override {
+    // Record all files we enter. We'll need them to diagnose headers without
+    // guards.
+    SourceManager &SM = PP->getSourceManager();
+    if (Reason == EnterFile && FileType == SrcMgr::C_User) {
+      if (const FileEntry *FE = SM.getFileEntryForID(SM.getFileID(Loc))) {
+        std::string FileName = cleanPath(FE->getName());
+        Files[FileName] = FE;
+      }
+    }
+  }
+
+  void Ifndef(SourceLocation Loc, const Token &MacroNameTok,
+              const MacroDirective *MD) override {
+    if (MD)
+      return;
+
+    // Record #ifndefs that succeeded. We also need the Location of the Name.
+    Ifndefs[MacroNameTok.getIdentifierInfo()] =
+        std::make_pair(Loc, MacroNameTok.getLocation());
+  }
+
+  void MacroDefined(const Token &MacroNameTok,
+                    const MacroDirective *MD) override {
+    // Record all defined macros. We store the whole token to get info on the
+    // name later.
+    Macros.emplace_back(MacroNameTok, MD);
+  }
+
+  void Endif(SourceLocation Loc, SourceLocation IfLoc) override {
+    // Record all #endif and the corresponding #ifs (including #ifndefs).
+    EndIfs[IfLoc] = Loc;
+  }
+
+  void EndOfMainFile() override {
+    // Now that we have all this information from the preprocessor, use it!
+    SourceManager &SM = PP->getSourceManager();
+
+    for (const auto &MacroEntry : Macros) {
+      const MacroInfo *MI = MacroEntry.second->getMacroInfo();
+
+      // We use clang's header guard detection. This has the advantage of also
+      // emitting a warning for cases where a pseudo header guard is found but
+      // preceeded by something blocking the header guard optimization.
+      if (!MI->isUsedForHeaderGuard())
+        continue;
+
+      const FileEntry *FE =
+          SM.getFileEntryForID(SM.getFileID(MI->getDefinitionLoc()));
+      std::string FileName = cleanPath(FE->getName());
+      Files.erase(FileName);
+
+      // See if we should check and fix this header guard.
+      if (!Check->shouldFixHeaderGuard(FileName))
+        continue;
+
+      // Look up Locations for this guard.
+      SourceLocation Ifndef =
+          Ifndefs[MacroEntry.first.getIdentifierInfo()].second;
+      SourceLocation Define = MacroEntry.first.getLocation();
+      SourceLocation EndIf =
+          EndIfs[Ifndefs[MacroEntry.first.getIdentifierInfo()].first];
+
+      // If the macro Name is not equal to what we can compute, correct it in
+      // the
+      // #ifndef and #define.
+      StringRef CurHeaderGuard =
+          MacroEntry.first.getIdentifierInfo()->getName();
+      std::string NewGuard =
+          checkHeaderGuardDefinition(Ifndef, Define, FileName, CurHeaderGuard);
+
+      // Now look at the #endif. We want a comment with the header guard. Fix it
+      // at the slightest deviation.
+      if (Check->shouldSuggestEndifComment(FileName))
+        checkEndifComment(EndIf, NewGuard);
+    }
+
+    // Emit warnings for headers that are missing guards.
+    checkGuardlessHeaders();
+  }
+
+  /// \brief Look for header guards that don't match the preferred style. Emit
+  /// fix-its and return the suggested header guard (or the original if no
+  /// change was made.
+  std::string checkHeaderGuardDefinition(SourceLocation Ifndef,
+                                         SourceLocation Define,
+                                         StringRef FileName,
+                                         StringRef CurHeaderGuard) {
+    std::string CPPVar = Check->getHeaderGuard(FileName, CurHeaderGuard);
+    std::string CPPVarUnder = CPPVar + '_'; // Allow a trailing underscore.
+    if (Ifndef.isValid() && CurHeaderGuard != CPPVar &&
+        CurHeaderGuard != CPPVarUnder) {
+      Check->diag(Ifndef, "header guard does not follow preferred style")
+          << FixItHint::CreateReplacement(
+                 CharSourceRange::getTokenRange(
+                     Ifndef, Ifndef.getLocWithOffset(CurHeaderGuard.size())),
+                 CPPVar)
+          << FixItHint::CreateReplacement(
+                 CharSourceRange::getTokenRange(
+                     Define, Define.getLocWithOffset(CurHeaderGuard.size())),
+                 CPPVar);
+      return CPPVar;
+    }
+    return CurHeaderGuard;
+  }
+
+  /// \brief Checks the comment after the #endif of a header guard and fixes it
+  /// if it doesn't match \c HeaderGuard.
+  void checkEndifComment(SourceLocation EndIf, StringRef HeaderGuard) {
+    const char *EndIfData = PP->getSourceManager().getCharacterData(EndIf);
+    size_t EndIfLen = std::strcspn(EndIfData, "\r\n");
+
+    StringRef EndIfStr(EndIfData, EndIfLen);
+    if (EndIf.isValid() && !EndIfStr.endswith("// " + HeaderGuard.str())) {
+      std::string Correct = "endif  // " + HeaderGuard.str();
+      Check->diag(EndIf, "#endif for a header guard should reference the "
+                         "guard macro in a comment")
+          << FixItHint::CreateReplacement(
+              CharSourceRange::getCharRange(EndIf,
+                                            EndIf.getLocWithOffset(EndIfLen)),
+              Correct);
+    }
+  }
+
+  /// \brief Looks for files that were visited but didn't have a header guard.
+  /// emits a warning with fixits suggesting adding one.
+  void checkGuardlessHeaders() {
+    // Look for header files that didn't have a header guard. Emit a warning and
+    // fix-its to add the guard.
+    // TODO: Insert the guard after top comments.
+    for (const auto &FE : Files) {
+      StringRef FileName = FE.getKey();
+      if (!Check->shouldSuggestToAddHeaderGuard(FileName))
+        continue;
+
+      SourceManager &SM = PP->getSourceManager();
+      FileID FID = SM.translateFile(FE.getValue());
+      SourceLocation StartLoc = SM.getLocForStartOfFile(FID);
+      if (StartLoc.isInvalid())
+        continue;
+
+      std::string CPPVar = Check->getHeaderGuard(FileName);
+      std::string CPPVarUnder = CPPVar + '_'; // Allow a trailing underscore.
+      // If there is a header guard macro but it's not in the topmost position
+      // emit a plain warning without fix-its. This often happens when the guard
+      // macro is preceeded by includes.
+      // FIXME: Can we move it into the right spot?
+      bool SeenMacro = false;
+      for (const auto &MacroEntry : Macros) {
+        StringRef Name = MacroEntry.first.getIdentifierInfo()->getName();
+        SourceLocation DefineLoc = MacroEntry.first.getLocation();
+        if ((Name == CPPVar || Name == CPPVarUnder) &&
+            SM.isWrittenInSameFile(StartLoc, DefineLoc)) {
+          Check->diag(
+              DefineLoc,
+              "Header guard after code/includes. Consider moving it up.");
+          SeenMacro = true;
+          break;
+        }
+      }
+
+      if (SeenMacro)
+        continue;
+
+      Check->diag(StartLoc, "header is missing header guard")
+          << FixItHint::CreateInsertion(
+                 StartLoc, "#ifndef " + CPPVar + "\n#define " + CPPVar + "\n\n")
+          << FixItHint::CreateInsertion(SM.getLocForEndOfFile(FID),
+                                        "\n#endif  // " + CPPVar + "\n");
+    }
+  }
+
+private:
+  std::vector<std::pair<Token, const MacroDirective *>> Macros;
+  llvm::StringMap<const FileEntry *> Files;
+  std::map<const IdentifierInfo *, std::pair<SourceLocation, SourceLocation>>
+      Ifndefs;
+  std::map<SourceLocation, SourceLocation> EndIfs;
+
+  Preprocessor *PP;
+  HeaderGuardCheck *Check;
+};
+} // namespace
+
+void HeaderGuardCheck::registerPPCallbacks(CompilerInstance &Compiler) {
+  Compiler.getPreprocessor().addPPCallbacks(
+      new HeaderGuardPPCallbacks(&Compiler.getPreprocessor(), this));
+}
+
+bool HeaderGuardCheck::shouldSuggestEndifComment(StringRef FileName) {
+  return FileName.endswith(".h");
+}
+
+bool HeaderGuardCheck::shouldFixHeaderGuard(StringRef FileName) { return true; }
+
+bool HeaderGuardCheck::shouldSuggestToAddHeaderGuard(StringRef FileName) {
+  return FileName.endswith(".h");
+}
+
+} // namespace tidy
+} // namespace clang

unittests/clang-tidy/LLVMModuleTest.cpp

 #include "ClangTidyTest.h"
+#include "llvm/HeaderGuardCheck.h"
 #include "llvm/IncludeOrderCheck.h"
 #include "llvm/NamespaceCommentCheck.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace tidy {
 namespace test {
 
EXPECT_EQ("namespace i { namespace jJ0_ {\n"
                 "namespace i { namespace jJ0_ {\n"
                 "} /* namespace qqq */ } /* random comment */"));
   EXPECT_EQ("namespace {\n"
             "} // namespace",
             runCheckOnCode<NamespaceCommentCheck>("namespace {\n"
                                                   "} // namespace asdf"));
 }
 
+static std::string runHeaderGuardCheck(StringRef Code, const Twine &Filename) {
+  return test::runCheckOnCode<LLVMHeaderGuardCheck>(
+      Code, /*Errors=*/nullptr, Filename, std::string("-xc++-header"));
+}
+
+TEST(LLVMHeaderGuardCheckTest, FixHeaderGuards) {
+  EXPECT_EQ("#ifndef LLVM_ADT_FOO_H\n#define LLVM_ADT_FOO_H\n#endif\n",
+            runHeaderGuardCheck("#ifndef FOO\n#define FOO\n#endif\n",
+                                "include/llvm/ADT/foo.h"));
+
+  // Allow trailing underscores.
+  EXPECT_EQ("#ifndef LLVM_ADT_FOO_H_\n#define LLVM_ADT_FOO_H_\n#endif\n",
+            runHeaderGuardCheck(
+                "#ifndef LLVM_ADT_FOO_H_\n#define LLVM_ADT_FOO_H_\n#endif\n",
+                "include/llvm/ADT/foo.h"));
+
+  EXPECT_EQ("#ifndef LLVM_CLANG_C_BAR_H\n#define LLVM_CLANG_C_BAR_H\n#endif\n",
+            runHeaderGuardCheck("", "./include/clang-c/bar.h"));
+
+  // No change expected here.
+  EXPECT_EQ("#ifndef FOO\n#define FOO\n#endif\n",
+            runHeaderGuardCheck("#ifndef FOO\n#define FOO\n#endif\n",
+                                "lib/internal/Bincludes.h"));
+
+  EXPECT_EQ(
+      "int foo;\n#ifndef LLVM_CLANG_BAR_H\n#define LLVM_CLANG_BAR_H\n#endif\n",
+      runHeaderGuardCheck("int foo;\n#ifndef LLVM_CLANG_BAR_H\n"
+                          "#define LLVM_CLANG_BAR_H\n#endif\n",
+                          "include/clang/bar.h"));
+
+  EXPECT_EQ("#ifndef LLVM_CLANG_BAR_H\n#define LLVM_CLANG_BAR_H\n"
+            "int foo;\n#ifndef FOOLOLO\n#define FOOLOLO\n#endif\n#endif\n",
+            runHeaderGuardCheck(
+                "int foo;\n#ifndef FOOLOLO\n#define FOOLOLO\n#endif\n",
+                "include/clang/bar.h"));
+}
+
 } // namespace test
 } // namespace tidy
 } // namespace clang