Fix llvm-header-guard check.

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)

Reviewers: djasper

Reviewed By: djasper

Subscribers: cfe-commits

Differential Revision: http://reviews.llvm.org/D5795

llvm-svn: 219789

clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp

@@ -150,15 +150,15 @@ public:
   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(' ');
@@ -166,8 +166,13 @@ public:
         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
@@ -207,11 +212,10 @@ public:
                          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)));
     }
   }
 
@@ -261,7 +265,7 @@ public:
           << FixItHint::CreateInsertion(
                  SM.getLocForEndOfFile(FID),
                  Check->shouldSuggestEndifComment(FileName)
-                     ? "\n#endif  // " + CPPVar + "\n"
+                     ? "\n#" + Check->formatEndIf(CPPVar) + "\n"
                      : "\n#endif\n");
     }
   }
@@ -294,5 +298,9 @@ bool HeaderGuardCheck::shouldSuggestToAddHeaderGuard(StringRef FileName) {
   return FileName.endswith(".h");
 }
 
+std::string HeaderGuardCheck::formatEndIf(StringRef HeaderGuard) {
+  return "endif // " + HeaderGuard.str();
+}
+
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/utils/HeaderGuard.h

@@ -32,6 +32,9 @@ public:
   /// \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;

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

@@ -103,9 +103,19 @@ TEST(LLVMHeaderGuardCheckTest, FixHeaderGuards) {
                                 "#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",
+            "#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",
             runHeaderGuardCheckWithEndif("#ifndef FOO\n"
                                          "#define FOO\n"
                                          "#endif\n",
@@ -114,7 +124,7 @@ TEST(LLVMHeaderGuardCheckTest, FixHeaderGuards) {
 
   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",
@@ -141,7 +151,7 @@ TEST(LLVMHeaderGuardCheckTest, FixHeaderGuards) {
 
   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",