[PATCH] Bug 27475 - Request header guard check processes .hpp files as well as .h files

[madsravn]

Summary

Patch for bug 27475: https://llvm.org/bugs/show_bug.cgi?id=27475

I used the suggestion of alexfh to check the header extension.

[hokein]

clang-tidy/llvm/HeaderGuardCheck.h

19 ↗

(On Diff #58044)

You should add a document for the option HeaderFileExtensions here.

clang-tidy/utils/HeaderGuard.cpp

300 ↗

(On Diff #58044)

To avoid the redundant code, you can put this kind of code to utls::HeaderFileExtensionsUtils (something like isHeaderFileExtension).

clang-tidy/utils/HeaderGuard.h

15 ↗

(On Diff #58044)

seems not needed?

45 ↗

(On Diff #58044)

Not needed to be a member variable.

[madsravn]

clang-tidy/llvm/HeaderGuardCheck.h

19 ↗

(On Diff #58044)

I'm not sure what you mean here. What is "a document" in this context?

[hokein]

clang-tidy/llvm/HeaderGuardCheck.h

19 ↗

(On Diff #58044)

Sorry for the unclear comment. You actually add an option HeaderFileExtensions in LLVMHeaderGuardCheck, so you need to add a document to describe it. See the file DefinitionsInHeadersCheck.h for details.

[alexfh]

clang-tidy/llvm/HeaderGuardCheck.cpp

18 ↗

(On Diff #58044)

This should be done in the constructor, not on each call to this method.

[hokein]

clang-tidy/llvm/HeaderGuardCheck.h

19 ↗

(On Diff #58044)

You need to add a comment here, like:

/// Finds and fixes header guards that do not adhere to LLVM style.
///
///  The check supports these options:
///   - `HeaderFileExtensions`: a comma-separated list of filename extensions of
///     header files (The filename extension should not contain "." prefix).
///     ",h,hh,hpp,hxx" by default.

[madsravn]

Updated the patch as suggested by hokein and alexfh.

[alexfh]

clang-tidy/llvm/HeaderGuardCheck.cpp

18 ↗

(On Diff #69131)

nit: In LLVM/Clang code it's not common to enclose single-line if/while/for bodies in braces.

18 ↗

(On Diff #69131)

if (extension.startswith("."))

22 ↗

(On Diff #69131)

return HeaderFileExtensions.count(extension) > 0;

26 ↗

(On Diff #69131)

Were you going to use isHeaderFileExtension, btw?

clang-tidy/utils/HeaderFileExtensionsUtils.cpp

64 ↗

(On Diff #69131)

1. clang-format the code
2. see comments for LLVMHeaderGuardCheck::shouldFixHeaderGuard above

[madsravn]

Fixed suggested by alexfh

[alexfh]

Thank you! I've just noticed that we had completely ignored the actual docs =\ See the inline comment for details.

clang-tidy/llvm/HeaderGuardCheck.h

20 ↗

(On Diff #69143)

Please add

/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/llvm-header-guard.html

and copy the rest of the comment to the corresponding .rst file with proper formatting (see docs/clang-tidy/checks/misc-argument-comment.rst for an example).

clang-tidy/utils/HeaderFileExtensionsUtils.h

44 ↗

(On Diff #69143)

nit: Add a trailing period.

[madsravn]

More suggestions by alexfh fixed.

[alexfh]

LG with a couple of nits. Do you need me to submit the patch for you? (If yes, I can fix the doc myself)

docs/clang-tidy/checks/llvm-header-guard.rst

8 ↗

(On Diff #69152)

This line is not needed here (it's a self-reference).

10 ↗

(On Diff #69152)

Should be

Options
-------

.. option:: HeaderFileExtensions

  ...

[madsravn]

Documentation fix.

[madsravn]

I have commit access. I can commit it myself :) thanks though.

Is the documentation correct? I wasn't sure about the "\n ..." part.

clang-tidy/utils/HeaderGuard.h

15 ↗

(On Diff #58044)

Needed for Options.

[alexfh]

docs/clang-tidy/checks/llvm-header-guard.rst

13 ↗

(On Diff #69158)

... was meant to represent the description of the option. Not literally ... ;)

The description should be indented by at least two columns and should start with A comma-separated ...

[madsravn]

docs/clang-tidy/checks/llvm-header-guard.rst

13 ↗

(On Diff #69158)

Sorry :) I just found the literal ... in some of the other .rst files and it somewhat fit the context. I'll get it fixed.

[alexfh]

docs/clang-tidy/checks/llvm-header-guard.rst

13 ↗

(On Diff #69164)

You missed the "The description should be indented by at least two columns" part.

With that should be fine to submit.

[madsravn]

Last change - documentation should be fine now.

clang-tools-extra/trunk/clang-tidy/llvm/HeaderGuardCheck.h

 
 #include "../utils/HeaderGuard.h"
 
 namespace clang {
 namespace tidy {
 namespace llvm {
 
 /// Finds and fixes header guards that do not adhere to LLVM style.
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/llvm-header-guard.html
+/// The check supports these options:
+///   - `HeaderFileExtensions`: a comma-separated list of filename extensions of
+///     header files (The filename extension should not contain "." prefix).
+///     ",h,hh,hpp,hxx" by default.
+///     For extension-less header files, using an empty string or leaving an
+///     empty string between "," if there are other filename extensions.
 class LLVMHeaderGuardCheck : public utils::HeaderGuardCheck {
 public:
   LLVMHeaderGuardCheck(StringRef Name, ClangTidyContext *Context)
-      : HeaderGuardCheck(Name, Context) {}
+      : HeaderGuardCheck(Name, Context),
+        RawStringHeaderFileExtensions(
+            Options.getLocalOrGlobal("HeaderFileExtensions", ",h,hh,hpp,hxx")) {
+    utils::parseHeaderFileExtensions(RawStringHeaderFileExtensions,
+                                     HeaderFileExtensions, ',');
+  }
   bool shouldSuggestEndifComment(StringRef Filename) override { return false; }
   bool shouldFixHeaderGuard(StringRef Filename) override;
   std::string getHeaderGuard(StringRef Filename, StringRef OldGuard) override;
+
+private:
+  std::string RawStringHeaderFileExtensions;
+  utils::HeaderFileExtensionsSet HeaderFileExtensions;
 };
 
 } // namespace llvm
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_HEADER_GUARD_CHECK_H

clang-tools-extra/trunk/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 {
 namespace llvm {
 
-bool LLVMHeaderGuardCheck::shouldFixHeaderGuard(StringRef Filename) {
-  return Filename.endswith(".h");
+bool LLVMHeaderGuardCheck::shouldFixHeaderGuard(StringRef FileName) {
+  return utils::isHeaderFileExtension(FileName, HeaderFileExtensions);
 }
 
 std::string LLVMHeaderGuardCheck::getHeaderGuard(StringRef Filename,
                                                  StringRef OldGuard) {
   std::string Guard = tooling::getAbsolutePath(Filename);
 
   // Sanitize the path. There are some rules for compatibility with the historic
   // style in include/llvm and include/clang which we want to preserve.

clang-tools-extra/trunk/clang-tidy/utils/HeaderFileExtensionsUtils.h

 //===--- HeaderFileExtensionsUtils.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_FILE_EXTENSIONS_UTILS_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_HEADER_FILE_EXTENSIONS_UTILS_H
 
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
-#include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/SmallSet.h"
+#include "llvm/ADT/StringRef.h"
 
 namespace clang {
 namespace tidy {
 namespace utils {
 
 typedef llvm::SmallSet<llvm::StringRef, 5> HeaderFileExtensionsSet;
 
 /// \brief Checks whether expansion location of \p Loc is in header file.
bool isSpellingLocInHeaderFile(
     SourceLocation Loc, SourceManager &SM,
     const HeaderFileExtensionsSet &HeaderFileExtensions);
 
 /// \brief Parses header file extensions from a semicolon-separated list.
 bool parseHeaderFileExtensions(StringRef AllHeaderFileExtensions,
                                HeaderFileExtensionsSet &HeaderFileExtensions,
                                char delimiter);
 
+/// \brief Decides whether a file has a header file extension.
+bool isHeaderFileExtension(StringRef FileName,
+                           HeaderFileExtensionsSet HeaderFileExtensions);
+
 } // namespace utils
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_HEADER_FILE_EXTENSIONS_UTILS_H

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

for (StringRef::const_iterator it = Extension.begin();
       if (!isAlphanumeric(*it))
         return false;
     }
     HeaderFileExtensions.insert(Extension);
   }
   return true;
 }
 
+bool isHeaderFileExtension(StringRef FileName,
+                           HeaderFileExtensionsSet HeaderFileExtensions) {
+  StringRef extension = ::llvm::sys::path::extension(FileName);
+  if (extension.startswith("."))
+    extension = extension.substr(1);
+
+  return HeaderFileExtensions.count(extension) > 0;
+}
+
 } // namespace utils
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/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_HEADERGUARD_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_HEADERGUARD_H
 
 #include "../ClangTidy.h"
+#include "../utils/HeaderFileExtensionsUtils.h"
 
 namespace clang {
 namespace tidy {
 namespace utils {
 
 /// Finds and fixes header guards.
+/// The check supports these options:
+///   - `HeaderFileExtensions`: a comma-separated list of filename extensions of
+///     header files (The filename extension should not contain "." prefix).
+///     ",h,hh,hpp,hxx" by default.
+///     For extension-less header files, using an empty string or leaving an
+///     empty string between "," if there are other filename extensions.
 class HeaderGuardCheck : public ClangTidyCheck {
 public:
   HeaderGuardCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+      : ClangTidyCheck(Name, Context),
+        RawStringHeaderFileExtensions(
+            Options.getLocalOrGlobal("HeaderFileExtensions", ",h,hh,hpp,hxx")) {
+    utils::parseHeaderFileExtensions(RawStringHeaderFileExtensions,
+                                     HeaderFileExtensions, ',');
+  }
   void registerPPCallbacks(CompilerInstance &Compiler) override;
 
   /// Returns ``true`` if the check should suggest inserting a trailing comment
   /// on the ``#endif`` of the header guard. It will use the same name as
   /// returned by ``HeaderGuardCheck::getHeaderGuard``.
   virtual bool shouldSuggestEndifComment(StringRef Filename);
   /// Returns ``true`` if the check should suggest changing an existing header
   /// guard to the string returned by ``HeaderGuardCheck::getHeaderGuard``.
   virtual bool shouldFixHeaderGuard(StringRef Filename);
   /// Returns ``true`` if the check should add a header guard to the file
   /// if it has none.
   virtual bool shouldSuggestToAddHeaderGuard(StringRef Filename);
   /// Returns a replacement for the ``#endif`` line with a comment mentioning
   /// \p HeaderGuard. The replacement should start with ``endif``.
   virtual std::string formatEndIf(StringRef HeaderGuard);
   /// Gets the canonical header guard for a file.
   virtual std::string getHeaderGuard(StringRef Filename,
                                      StringRef OldGuard = StringRef()) = 0;
+
+private:
+  std::string RawStringHeaderFileExtensions;
+  utils::HeaderFileExtensionsSet HeaderFileExtensions;
 };
 
 } // namespace utils
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_HEADERGUARD_H

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

 #include "llvm/Support/Path.h"
 
 namespace clang {
 namespace tidy {
 namespace utils {
 
 /// \brief canonicalize a path by removing ./ and ../ components.
 static std::string cleanPath(StringRef Path) {
-  SmallString<256> Result =  Path;
+  SmallString<256> Result = Path;
   llvm::sys::path::remove_dots(Result, true);
   return Result.str();
 }
 
 namespace {
 class HeaderGuardPPCallbacks : public PPCallbacks {
 public:
   HeaderGuardPPCallbacks(Preprocessor *PP, HeaderGuardCheck *Check)

 
 void HeaderGuardCheck::registerPPCallbacks(CompilerInstance &Compiler) {
   Compiler.getPreprocessor().addPPCallbacks(
       llvm::make_unique<HeaderGuardPPCallbacks>(&Compiler.getPreprocessor(),
                                                 this));
 }
 
 bool HeaderGuardCheck::shouldSuggestEndifComment(StringRef FileName) {
-  return FileName.endswith(".h");
+  return utils::isHeaderFileExtension(FileName, HeaderFileExtensions);
 }
 
 bool HeaderGuardCheck::shouldFixHeaderGuard(StringRef FileName) { return true; }
 
 bool HeaderGuardCheck::shouldSuggestToAddHeaderGuard(StringRef FileName) {
-  return FileName.endswith(".h");
+  return utils::isHeaderFileExtension(FileName, HeaderFileExtensions);
 }
 
 std::string HeaderGuardCheck::formatEndIf(StringRef HeaderGuard) {
   return "endif // " + HeaderGuard.str();
 }
 
 } // namespace utils
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/docs/clang-tidy/checks/llvm-header-guard.rst

 .. title:: clang-tidy - llvm-header-guard
 
 llvm-header-guard
 =================
 
-TODO: add docs
+Finds and fixes header guards that do not adhere to LLVM style.
+
+Options
+-------
+
+.. option:: HeaderFileExtensions
+
+  A comma-separated list of filename extensions of header files (The filename extension should not contain "." prefix). Default value is ",h,hh,hpp,hxx".  For extension-less header files, using an empty string or leaving an empty string between "," if there are other filename extensions.