[clang-tidy] Reword the "code outside header guard" warning.

[bkramer]

Summary

The check doesn't really know if the code it is warning about came before
or after the header guard, so phrase it more neutral instead of complaining
about code before the header guard. The location for the warning is still
not optimal, but I don't think fixing that is worth the effort, the
preprocessor doesn't give us a better location.

[alexfh]

LG. Thank you for the fix!

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

for (const auto &FE : Files) {
       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.
+      // If there's a macro with a name that follows the header guard convention
+      // but was not recognized by the preprocessor as a header guard there must
+      // be code outside of the guarded area. Emit a plain warning without
+      // fix-its.
       // 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.");
+          Check->diag(DefineLoc, "code/includes outside of area guarded by "
+                                 "header guard; consider moving it");
           SeenMacro = true;
           break;
         }
       }
 
       if (SeenMacro)
         continue;
 

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

 #include "ClangTidyTest.h"
 #include "llvm/HeaderGuardCheck.h"
 #include "llvm/IncludeOrderCheck.h"
 #include "gtest/gtest.h"
 
 using namespace clang::tidy::llvm;
 
 namespace clang {
 namespace tidy {
 namespace test {
 
 // FIXME: It seems this might be incompatible to dos path. Investigating.
 #if !defined(_WIN32)
 static std::string runHeaderGuardCheck(StringRef Code, const Twine &Filename,
-                                       unsigned ExpectedWarnings) {
+                                       Optional<StringRef> ExpectedWarning) {
   std::vector<ClangTidyError> Errors;
   std::string Result = test::runCheckOnCode<LLVMHeaderGuardCheck>(
       Code, &Errors, Filename, std::string("-xc++-header"));
-  return Errors.size() == ExpectedWarnings ? Result : "invalid error count";
+  if (Errors.size() != (size_t)ExpectedWarning.hasValue())
+    return "invalid error count";
+  if (ExpectedWarning && *ExpectedWarning != Errors.back().Message.Message)
+    return "expected: '" + ExpectedWarning->str() + "', saw: '" +
+           Errors.back().Message.Message + "'";
+  return Result;
 }
 
 namespace {
 struct WithEndifComment : public LLVMHeaderGuardCheck {
   WithEndifComment(StringRef Name, ClangTidyContext *Context)
       : LLVMHeaderGuardCheck(Name, Context) {}
   bool shouldSuggestEndifComment(StringRef Filename) override { return true; }
 };
 } // namespace
 
-static std::string runHeaderGuardCheckWithEndif(StringRef Code,
-                                                const Twine &Filename,
-                                                unsigned ExpectedWarnings) {
+static std::string
+runHeaderGuardCheckWithEndif(StringRef Code, const Twine &Filename,
+                             Optional<StringRef> ExpectedWarning) {
   std::vector<ClangTidyError> Errors;
   std::string Result = test::runCheckOnCode<WithEndifComment>(
       Code, &Errors, Filename, std::string("-xc++-header"));
-  return Errors.size() == ExpectedWarnings ? Result : "invalid error count";
+  if (Errors.size() != (size_t)ExpectedWarning.hasValue())
+    return "invalid error count";
+  if (ExpectedWarning && *ExpectedWarning != Errors.back().Message.Message)
+    return "expected: '" + ExpectedWarning->str() + "', saw: '" +
+           Errors.back().Message.Message + "'";
+  return Result;
 }
 
 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",
-                                /*ExpectedWarnings=*/1));
+            runHeaderGuardCheck(
+                "#ifndef FOO\n"
+                StringRef("header guard does not follow preferred style")));
 
   // 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",
-                                /*ExpectedWarnings=*/0));
+                                "include/llvm/ADT/foo.h", None));
 
   EXPECT_EQ("#ifndef LLVM_CLANG_C_BAR_H\n"
             "#define LLVM_CLANG_C_BAR_H\n"
             "\n"
             "\n"
             "#endif\n",
             runHeaderGuardCheck("", "./include/clang-c/bar.h",
-                                /*ExpectedWarnings=*/1));
+                                StringRef("header is missing header guard")));
 
   EXPECT_EQ("#ifndef LLVM_CLANG_LIB_CODEGEN_C_H\n"
             "#define LLVM_CLANG_LIB_CODEGEN_C_H\n"
             "\n"
             "\n"
             "#endif\n",
             runHeaderGuardCheck("", "tools/clang/lib/CodeGen/c.h",
-                                /*ExpectedWarnings=*/1));
+                                StringRef("header is missing header guard")));
 
   EXPECT_EQ("#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_X_H\n"
             "#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_X_H\n"
             "\n"
             "\n"
             "#endif\n",
             runHeaderGuardCheck("", "tools/clang/tools/extra/clang-tidy/x.h",
-                                /*ExpectedWarnings=*/1));
+                                StringRef("header is missing header guard")));
 
-  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", /*ExpectedWarnings=*/1));
+  EXPECT_EQ(
+      "int foo;\n"
+                          "include/clang/bar.h",
+                          StringRef("code/includes outside of area guarded by "
+                                    "header guard; consider moving it")));
+
+  EXPECT_EQ(
+      "#ifndef LLVM_CLANG_BAR_H\n"
+      "#define LLVM_CLANG_BAR_H\n"
+      "#endif\n"
+      "int foo;\n",
+      runHeaderGuardCheck("#ifndef LLVM_CLANG_BAR_H\n"
+                          "#define LLVM_CLANG_BAR_H\n"
+                          "#endif\n"
+                          "int foo;\n",
+                          "include/clang/bar.h",
+                          StringRef("code/includes outside of area guarded by "
+                                    "header guard; consider moving it")));
 
   EXPECT_EQ("#ifndef LLVM_CLANG_BAR_H\n"
             "#define LLVM_CLANG_BAR_H\n"
             "\n"
             "int foo;\n"
             "#ifndef FOOLOLO\n"
             "#define FOOLOLO\n"
             "#endif\n"
             "\n"
             "#endif\n",
             runHeaderGuardCheck("int foo;\n"
                                 "#ifndef FOOLOLO\n"
                                 "#define FOOLOLO\n"
                                 "#endif\n",
-                                "include/clang/bar.h", /*ExpectedWarnings=*/1));
+                                "include/clang/bar.h",
+                                StringRef("header is missing header guard")));
 
   // 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));
+            runHeaderGuardCheck(
+                "#ifndef FOO\n"
+                StringRef("header guard does not follow preferred style")));
 
   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",
-                                         "include/llvm/ADT/foo.h",
-                                         /*ExpectedWarnings=*/1));
+            runHeaderGuardCheckWithEndif(
+                "#ifndef FOO\n"
+                StringRef("header guard does not follow preferred style")));
 
   EXPECT_EQ("#ifndef LLVM_ADT_FOO_H\n"
             "#define 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));
+            runHeaderGuardCheckWithEndif(
+                "#ifndef LLVM_ADT_FOO_H\n"
+                StringRef("#endif for a header guard should reference the "
+                          "guard macro in a comment")));
 
   EXPECT_EQ("#ifndef LLVM_ADT_FOO_H\n"
             "#define 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_ADT_FOO_H */\n",
-                                         "include/llvm/ADT/foo.h",
-                                         /*ExpectedWarnings=*/0));
+                                         "include/llvm/ADT/foo.h", None));
 
   EXPECT_EQ("#ifndef LLVM_ADT_FOO_H_\n"
             "#define 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_ADT_FOO_H_\n",
-                                         "include/llvm/ADT/foo.h",
-                                         /*ExpectedWarnings=*/0));
+                                         "include/llvm/ADT/foo.h", None));
 
   EXPECT_EQ("#ifndef LLVM_ADT_FOO_H\n"
             "#define 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));
+            runHeaderGuardCheckWithEndif(
+                "#ifndef LLVM_ADT_FOO_H_\n"
+                StringRef("header guard does not follow preferred style")));
 
   EXPECT_EQ("#ifndef LLVM_ADT_FOO_H\n"
             "#define LLVM_ADT_FOO_H\n"
             "#endif \\ \n"
             "// LLVM_ADT_FOO_H\n",
-            runHeaderGuardCheckWithEndif("#ifndef LLVM_ADT_FOO_H\n"
-                                         "#define LLVM_ADT_FOO_H\n"
-                                         "#endif \\ \n"
-                                         "// LLVM_ADT_FOO_H\n",
-                                         "include/llvm/ADT/foo.h",
-                                         /*ExpectedWarnings=*/1));
+            runHeaderGuardCheckWithEndif(
+                "#ifndef LLVM_ADT_FOO_H\n"
+                StringRef("backslash and newline separated by space")));
 
   EXPECT_EQ("#ifndef LLVM_ADT_FOO_H\n"
             "#define LLVM_ADT_FOO_H\n"
             "#endif  /* LLVM_ADT_FOO_H\\ \n"
             " FOO */",
             runHeaderGuardCheckWithEndif("#ifndef LLVM_ADT_FOO_H\n"
                                          "#define LLVM_ADT_FOO_H\n"
                                          "#endif  /* LLVM_ADT_FOO_H\\ \n"
                                          " FOO */",
-                                         "include/llvm/ADT/foo.h",
-                                         /*ExpectedWarnings=*/0));
+                                         "include/llvm/ADT/foo.h", None));
 }
 #endif
 
 } // namespace test
 } // namespace tidy
 } // namespace clang