[PATCH] [clang-tidy] Add a generic header guard checker + LLVM implementation.
Manuel Klimek
klimek at google.com
Wed Aug 13 02:46:03 PDT 2014
================
Comment at: clang-tidy/llvm/HeaderGuardCheck.cpp:16
@@ +15,3 @@
+bool LLVMHeaderGuardCheck::shouldFixHeaderGuard(StringRef Filename) {
+ return Filename.find("include") != StringRef::npos;
+}
----------------
Is it intentional that this finds lib/internal/Bincludes.h?
================
Comment at: clang-tidy/llvm/HeaderGuardCheck.h:21
@@ +20,3 @@
+ public:
+ bool shouldSuggestEndifComment(StringRef Filename) { return false; }
+ bool shouldFixHeaderGuard(StringRef Filename) override;
----------------
You might want to run the override clang-tidy checker :P
================
Comment at: clang-tidy/utils/HeaderGuard.cpp:20-21
@@ +19,4 @@
+
+/// \brief canonicalize a path by removing ./ and ../ components.
+static std::string cleanPath(StringRef Path) {
+ SmallString<256> NewPath;
----------------
Perhaps add a FIXME to put this somewhere more central.
================
Comment at: clang-tidy/utils/HeaderGuard.cpp:29
@@ +28,3 @@
+ // Drop the last component.
+ NewPath.resize(llvm::sys::path::parent_path(NewPath).size());
+ } else {
----------------
Do we want an error for something like cleanPath("../../../../some/path")?
================
Comment at: clang-tidy/utils/HeaderGuard.cpp:71-72
@@ +70,4 @@
+ const MacroDirective *MD) override {
+ // Record all Defines macros. We store the whole token to get info on the
+ // Name later.
+ Macros.emplace_back(MacroNameTok, MD);
----------------
s/Defines/defined/; s/Name/name/
================
Comment at: clang-tidy/utils/HeaderGuard.cpp:103-126
@@ +102,26 @@
+
+ // 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 Name = MacroEntry.first.getIdentifierInfo()->getName();
+ std::string CPPVar = Check->getHeaderGuard(FileName, Name);
+ std::string CPPVarUnder = CPPVar + '_'; // Allow a trailing underscore.
+ if (ifndef.isValid() && Name != CPPVar && Name != CPPVarUnder) {
+ Check->diag(ifndef, "header guard does not follow preferred style")
+ << FixItHint::CreateReplacement(
+ CharSourceRange::getTokenRange(
+ ifndef, ifndef.getLocWithOffset(Name.size())),
+ CPPVar)
+ << FixItHint::CreateReplacement(
+ CharSourceRange::getTokenRange(
+ Define, Define.getLocWithOffset(Name.size())),
+ CPPVar);
+ Name = CPPVar;
+ }
+
----------------
Pull this into an extra method.
================
Comment at: clang-tidy/utils/HeaderGuard.cpp:104-105
@@ +103,4 @@
+ // Look up Locations for this guard.
+ SourceLocation ifndef =
+ Ifndefs[MacroEntry.first.getIdentifierInfo()].second;
+ SourceLocation Define = MacroEntry.first.getLocation();
----------------
s/ifndef/Ifndef/
================
Comment at: clang-tidy/utils/HeaderGuard.cpp:110-111
@@ +109,4 @@
+
+ // If the macro Name is not equal to what we can compute correct it in the
+ // #ifndef and #define.
+ StringRef Name = MacroEntry.first.getIdentifierInfo()->getName();
----------------
s/compute/compute, /
================
Comment at: clang-tidy/utils/HeaderGuard.cpp:131-145
@@ +130,17 @@
+
+ // Now look at the #endif. We want a comment with the header guard. Fix it
+ // at the slightest deviation.
+ const char *EndIfData = SM.getCharacterData(EndIf);
+ size_t EndIfLen = std::strcspn(EndIfData, "\r\n");
+
+ StringRef EndIfStr(EndIfData, EndIfLen);
+ if (EndIf.isValid() && !EndIfStr.endswith("// " + Name.str())) {
+ std::string Correct = "endif // " + Name.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);
+ }
+ }
----------------
Pull this into an extra method.
================
Comment at: clang-tidy/utils/HeaderGuard.cpp:148-189
@@ +147,44 @@
+
+ // 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;
+
+ FileID FID = SM.translateFile(FE.getValue());
+ SourceLocation StartLoc = SM.getLocForStartOfFile(FID);
+ if (StartLoc.isInvalid())
+ continue;
+
+ std::string CPPVar = Check->getHeaderGuard(FileName);
+ // 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.
+ // TODO: 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();
+ std::string CPPVarUnder = CPPVar + '_'; // Allow a trailing underscore.
+ 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");
+ }
+ }
----------------
Pull this into an extra method.
================
Comment at: clang-tidy/utils/HeaderGuard.cpp:165
@@ +164,3 @@
+ // macro is preceeded by includes.
+ // TODO: Can we move it into the right spot?
+ bool SeenMacro = false;
----------------
s/TODO/FIXME/
================
Comment at: clang-tidy/utils/HeaderGuard.cpp:170
@@ +169,3 @@
+ SourceLocation DefineLoc = MacroEntry.first.getLocation();
+ std::string CPPVarUnder = CPPVar + '_'; // Allow a trailing underscore.
+ if ((Name == CPPVar || Name == CPPVarUnder) &&
----------------
Any reason this an CPPVarUnder are not declared next to each other?
================
Comment at: clang-tidy/utils/HeaderGuard.h:23
@@ +22,3 @@
+
+ /// \brief return true if the checker should suggest inserting a trailing
+ /// comment on the #endif of the header guard. It will use the same name as
----------------
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.
================
Comment at: clang-tidy/utils/HeaderGuard.h:27-28
@@ +26,4 @@
+ virtual bool shouldSuggestEndifComment(StringRef Filename);
+ /// \brief return true if the checker should suggest changing a header guard
+ /// the string returned by getHeaderGuard.
+ virtual bool shouldFixHeaderGuard(StringRef Filename);
----------------
Can't parse the sentence.
================
Comment at: unittests/clang-tidy/LLVMModuleTest.cpp:94-101
@@ +93,10 @@
+
+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"));
+
+ EXPECT_EQ("#ifndef LLVM_CLANG_C_BAR_H\n#define LLVM_CLANG_C_BAR_H\n#endif\n",
+ runHeaderGuardCheck("", "./include/clang-c/bar.h"));
+}
+
----------------
I somehow don't believe these are enough tests :)
http://reviews.llvm.org/D4867
More information about the cfe-commits
mailing list