Fix llvm-header-guard check.

[alexfh]

Summary

This patch makes the check work better for LLVM code:

• always fix existing #endif comments
• use one space before the comment (+allow customization for other styles)

[djasper]

Looks good.

clang-tidy/utils/HeaderGuard.h

public:
   /// 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 Returns a replacement for endif line with a comment mentioning
+  /// \p HeaderGuard. The replacement should start with "endif".
+  virtual std::string formatEndIf(StringRef HeaderGuard);
   /// \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

void EndOfMainFile() override {
     Files.clear();
     Ifndefs.clear();
     EndIfs.clear();
   }
 
   bool wouldFixEndifComment(StringRef FileName, SourceLocation EndIf,
                             StringRef HeaderGuard,
                             size_t *EndIfLenPtr = nullptr) {
-    if (!Check->shouldSuggestEndifComment(FileName))
+    if (!EndIf.isValid())
       return false;
-
     const char *EndIfData = PP->getSourceManager().getCharacterData(EndIf);
     size_t EndIfLen = std::strcspn(EndIfData, "\r\n");
     if (EndIfLenPtr)
       *EndIfLenPtr = EndIfLen;
 
     StringRef EndIfStr(EndIfData, EndIfLen);
+    EndIfStr = EndIfStr.substr(EndIfStr.find_first_not_of("#endif \t"));
 
     // Give up if there's an escaped newline.
     size_t FindEscapedNewline = EndIfStr.find_last_not_of(' ');
     if (FindEscapedNewline != StringRef::npos &&
         EndIfStr[FindEscapedNewline] == '\\')
       return false;
 
-    return (EndIf.isValid() && !EndIfStr.endswith("// " + HeaderGuard.str()) &&
-            !EndIfStr.endswith("/* " + HeaderGuard.str() + " */"));
+    if (!Check->shouldSuggestEndifComment(FileName) &&
+        !(EndIfStr.startswith("//") ||
+          (EndIfStr.startswith("/*") && EndIfStr.endswith("*/"))))
+      return false;
+
+    return (EndIfStr != "// " + HeaderGuard.str()) &&
+           (EndIfStr != "/* " + HeaderGuard.str() + " */");
   }
 
   /// \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,
                                          SourceLocation EndIf,
public:
 
   /// \brief Checks the comment after the #endif of a header guard and fixes it
   /// if it doesn't match \c HeaderGuard.
   void checkEndifComment(StringRef FileName, SourceLocation EndIf,
                          StringRef HeaderGuard,
                          std::vector<FixItHint> &FixIts) {
     size_t EndIfLen;
     if (wouldFixEndifComment(FileName, EndIf, HeaderGuard, &EndIfLen)) {
-      std::string Correct = "endif  // " + HeaderGuard.str();
       FixIts.push_back(FixItHint::CreateReplacement(
           CharSourceRange::getCharRange(EndIf,
                                         EndIf.getLocWithOffset(EndIfLen)),
-          Correct));
+          Check->formatEndIf(HeaderGuard)));
     }
   }
 
   /// \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.
for (const auto &FE : Files) {
         continue;
 
       Check->diag(StartLoc, "header is missing header guard")
           << FixItHint::CreateInsertion(
                  StartLoc, "#ifndef " + CPPVar + "\n#define " + CPPVar + "\n\n")
           << FixItHint::CreateInsertion(
                  SM.getLocForEndOfFile(FID),
                  Check->shouldSuggestEndifComment(FileName)
-                     ? "\n#endif  // " + CPPVar + "\n"
+                     ? "\n#" + Check->formatEndIf(CPPVar) + "\n"
                      : "\n#endif\n");
     }
   }
 
 private:
   std::vector<std::pair<Token, const MacroDirective *>> Macros;
   llvm::StringMap<const FileEntry *> Files;
   std::map<const IdentifierInfo *, std::pair<SourceLocation, SourceLocation>>

 }
 
 bool HeaderGuardCheck::shouldFixHeaderGuard(StringRef FileName) { return true; }
 
 bool HeaderGuardCheck::shouldSuggestToAddHeaderGuard(StringRef FileName) {
   return FileName.endswith(".h");
 }
 
+std::string HeaderGuardCheck::formatEndIf(StringRef HeaderGuard) {
+  return "endif // " + HeaderGuard.str();
+}
+
 } // namespace tidy
 } // namespace clang

unittests/clang-tidy/LLVMModuleTest.cpp

EXPECT_EQ("#ifndef LLVM_CLANG_BAR_H\n"
             "\n"
             "#endif\n",
             runHeaderGuardCheck("int foo;\n"
                                 "#ifndef FOOLOLO\n"
                                 "#define FOOLOLO\n"
                                 "#endif\n",
                                 "include/clang/bar.h", /*ExpectedWarnings=*/1));
 
+  // Fix incorrect #endif comments even if we shouldn't add new ones.
+  EXPECT_EQ("#ifndef LLVM_ADT_FOO_H\n"
+            "#define LLVM_ADT_FOO_H\n"
+            "#endif // LLVM_ADT_FOO_H\n",
+            runHeaderGuardCheck("#ifndef FOO\n"
+                                "#define FOO\n"
+                                "#endif // FOO\n",
+                                "include/llvm/ADT/foo.h",
+                                /*ExpectedWarnings=*/1));
+
   EXPECT_EQ("#ifndef LLVM_ADT_FOO_H\n"
             "#define LLVM_ADT_FOO_H\n"
-            "#endif  // LLVM_ADT_FOO_H\n",
+            "#endif // LLVM_ADT_FOO_H\n",
             runHeaderGuardCheckWithEndif("#ifndef FOO\n"
                                          "#define FOO\n"
                                          "#endif\n",
                                          "include/llvm/ADT/foo.h",
                                          /*ExpectedWarnings=*/1));
 
   EXPECT_EQ("#ifndef LLVM_ADT_FOO_H\n"
             "#define LLVM_ADT_FOO_H\n"
-            "#endif  // LLVM_ADT_FOO_H\n",
+            "#endif // LLVM_ADT_FOO_H\n",
             runHeaderGuardCheckWithEndif("#ifndef LLVM_ADT_FOO_H\n"
                                          "#define LLVM_ADT_FOO_H\n"
                                          "#endif // LLVM_H\n",
                                          "include/llvm/ADT/foo.h",
                                          /*ExpectedWarnings=*/1));
 
   EXPECT_EQ("#ifndef LLVM_ADT_FOO_H\n"
             "#define LLVM_ADT_FOO_H\n"
EXPECT_EQ("#ifndef LLVM_ADT_FOO_H_\n"
             runHeaderGuardCheckWithEndif("#ifndef LLVM_ADT_FOO_H_\n"
                                          "#define LLVM_ADT_FOO_H_\n"
                                          "#endif // LLVM_ADT_FOO_H_\n",
                                          "include/llvm/ADT/foo.h",
                                          /*ExpectedWarnings=*/0));
 
   EXPECT_EQ("#ifndef LLVM_ADT_FOO_H\n"
             "#define LLVM_ADT_FOO_H\n"
-            "#endif  // LLVM_ADT_FOO_H\n",
+            "#endif // LLVM_ADT_FOO_H\n",
             runHeaderGuardCheckWithEndif("#ifndef LLVM_ADT_FOO_H_\n"
                                          "#define LLVM_ADT_FOO_H_\n"
                                          "#endif // LLVM\n",
                                          "include/llvm/ADT/foo.h",
                                          /*ExpectedWarnings=*/1));
 
   EXPECT_EQ("#ifndef LLVM_ADT_FOO_H\n"
             "#define LLVM_ADT_FOO_H\n"