[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

You should add a document for the option HeaderFileExtensions here.

clang-tidy/utils/HeaderGuard.cpp

300

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

clang-tidy/utils/HeaderGuard.h

15

seems not needed?

45

Not needed to be a member variable.

[madsravn]

clang-tidy/llvm/HeaderGuardCheck.h

19

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

[hokein]

clang-tidy/llvm/HeaderGuardCheck.h

19

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

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

[hokein]

clang-tidy/llvm/HeaderGuardCheck.h

19

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

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

18

if (extension.startswith("."))

22

return HeaderFileExtensions.count(extension) > 0;

31

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

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

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-tidy/llvm/HeaderGuardCheck.h

 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_HEADER_GUARD_CHECK_H
 
 #include "../utils/HeaderGuard.h"
 
 namespace clang {
 namespace tidy {
 namespace llvm {
 
 /// Finds and fixes header guards that do not adhere to LLVM style.
 class LLVMHeaderGuardCheck : public utils::HeaderGuardCheck {
 public:
   LLVMHeaderGuardCheck(StringRef Name, ClangTidyContext *Context)
       : HeaderGuardCheck(Name, Context) {}
   bool shouldSuggestEndifComment(StringRef Filename) override { return false; }
   bool shouldFixHeaderGuard(StringRef Filename) override;
   std::string getHeaderGuard(StringRef Filename, StringRef OldGuard) override;
+private:
+  utils::HeaderFileExtensionsSet HeaderFileExtensions;
 };
 
 } // namespace llvm
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_HEADER_GUARD_CHECK_H

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");
+  const std::string RawStringHeaderFileExtensions(
+              Options.getLocalOrGlobal("HeaderFileExtensions", ",h,hh,hpp,hxx"));
+  utils::parseHeaderFileExtensions(RawStringHeaderFileExtensions,
+                                          HeaderFileExtensions,
+                                          ',');
+  StringRef extension = ::llvm::sys::path::extension(Filename);
+  if (extension.size() > 0 && extension.front() == '.') {
+    extension = extension.substr(1);
+  }
+
+  if (HeaderFileExtensions.count(extension))
+    return true;
+
+  return false;
 }
 
 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-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"
+#include "llvm/Support/Path.h"
 
 namespace clang {
 namespace tidy {
 namespace utils {
 
 /// Finds and fixes header guards.
 class HeaderGuardCheck : public ClangTidyCheck {
 public:
public:
   /// 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;
+private:
+  utils::HeaderFileExtensionsSet HeaderFileExtensions;
 };
 
 } // namespace utils
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_HEADERGUARD_H

clang-tidy/utils/HeaderGuard.cpp

 
 void HeaderGuardCheck::registerPPCallbacks(CompilerInstance &Compiler) {
   Compiler.getPreprocessor().addPPCallbacks(
       llvm::make_unique<HeaderGuardPPCallbacks>(&Compiler.getPreprocessor(),
                                                 this));
 }
 
 bool HeaderGuardCheck::shouldSuggestEndifComment(StringRef FileName) {
-  return FileName.endswith(".h");
+  const std::string RawStringHeaderFileExtensions(
+                Options.getLocalOrGlobal("HeaderFileExtensions", ",h,hh,hpp,hxx"));
+  utils::parseHeaderFileExtensions(RawStringHeaderFileExtensions,
+                                            HeaderFileExtensions,
+                                            ',');
+  StringRef extension = ::llvm::sys::path::extension(FileName);
+  if (extension.size() > 0 && extension.front() == '.') {
+    extension = extension.substr(1);
+  }
+  if (HeaderFileExtensions.count(extension))
+    return true;
+
+  return false;
 }
 
 bool HeaderGuardCheck::shouldFixHeaderGuard(StringRef FileName) { return true; }
 
 bool HeaderGuardCheck::shouldSuggestToAddHeaderGuard(StringRef FileName) {
-  return FileName.endswith(".h");
+  const std::string RawStringHeaderFileExtensions(
+                Options.getLocalOrGlobal("HeaderFileExtensions", ",h,hh,hpp,hxx"));
+  utils::parseHeaderFileExtensions(RawStringHeaderFileExtensions,
+                                            HeaderFileExtensions,
+                                            ',');
+  StringRef extension = ::llvm::sys::path::extension(FileName);
+  if (extension.size() > 0 && extension.front() == '.') {
+    extension = extension.substr(1);
+  }
+  if (HeaderFileExtensions.count(extension))
+    return true;
+
+  return false;
 }
 
 std::string HeaderGuardCheck::formatEndIf(StringRef HeaderGuard) {
   return "endif // " + HeaderGuard.str();
 }
 
 } // namespace utils
 } // namespace tidy
 } // namespace clang