[clang-tidy] bugprone-header-guard : a simple version of llvm-header-guard

[trixirt]

Summary

A general use, missing header guard finder and fixer.
No style checks. If there is already a working header guard, it is used.
If the header guard is missing or broken, it is created from the path name
using this method.

<path-to>/foo.h ->

#ifndef FOO_H
#define FOO_H

#endif

The motivation for this change is fixing a large, non llvm with many missing header guards.
As no style is checked, it should not conflict with llvm-header-guard.

Fix docs formatting of all checks using with the HeaderFileExtensions option.

[Eugene.Zelenko]

clang-tidy/misc/HeaderGuardCheck.cpp

21 ↗

(On Diff #198001)

Please use early return.

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

14 ↗

(On Diff #198001)

Please use single back-tick instead of quotes for option values. Same below.

[trixirt]

Addressed issue with returning early and using backtick in doc.
Since misc-header-guard doc was a cut-n-paste of llvm-header-guard, that and other similar docs were changed.

[JonasToth]

Hi trixirt and thanks for the patch!

I would rather like to generalize the llvm check to allow different styles and then alias the general version with different configurations. Introducing this code duplication does not sound like a good idea to me.
The documentation fixes you make can be done in a separate patch to keep things clean.

[aaron.ballman]

In D61508#1491570, @JonasToth wrote:

Hi trixirt and thanks for the patch!

I would rather like to generalize the llvm check to allow different styles and then alias the general version with different configurations. Introducing this code duplication does not sound like a good idea to me.
The documentation fixes you make can be done in a separate patch to keep things clean.

I'd probably reverse that -- have bugprone (not misc) carry a general check for header guards with configuration options, and have the llvm check defer to the general check with specific configuration options.

clang-tidy/misc/HeaderGuardCheck.cpp

25–26 ↗

(On Diff #198146)

This replacement may generate a header guard that exhibits UB. Consider a filename like foo-.cpp, which will become foo__cpp

[trixirt]

Move from misc to bugprone

[trixirt]

All of the real work is the header-guard checks is done by their common base class utils/HeaderGuard.
The much smaller llvm and bugprone subclasses are only concerned with the style of the fix.

My understanding of the directory layout guidance in the docs is that different styles get different directories.

Next, you need to decide which module the check belongs to. Modules
are located in subdirectories of `clang-tidy/
<https://github.com/llvm/llvm-project/tree/master/clang-tools-extra/clang-tidy/>`_
and contain checks targeting a certain aspect of code quality (performance,
readability, etc.), certain coding style or standard (Google, LLVM, CERT, etc.)
or a widely used API (e.g. MPI). Their names are same as user-facing check
groups names described :ref:`above <checks-groups-table>`.

So keep as-is ?

[JonasToth]

My understanding of the directory layout guidance in the docs is that different styles get different directories.

Yes, but for "the same check" we use aliases instead. When aliasing checks in different categories it is possible to change default configuration values, so we keep the code in one check and do the rest with options.

[trixirt]

llvm-header-guard is an alias to bugprone-header-guard.
The style is passed through the option 'GuardStyle'

A regression test was added for llvm-header-guard.
llvm unit tests were modified to the new class and option interface.

[aaron.ballman]

clang-tidy/bugprone/HeaderGuardCheck.cpp

51

I think this should be an else if rather than an else. I'd like to see us diagnose unknown guard styles so that we only accept "llvm" and "" as guard styles; this makes it easier to add new guard styles in the future.

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

12

Should we be documenting the GuardStyle option here as well? We could document "llvm" as the only available option currently and mention the llvm-header-guard check as an alias which enables this style.

test/clang-tidy/bugprone-header-guard.cpp

2–4

Can you add a test for the GuardStyle option? Both when specifying llvm and some random unsupported string.

[trixirt]

Change the GuardStyle data type from a string to an enum.
Add test cases for explicitly setting valid style.
Document valid style values.
Reduce llvm-header-guard doc to point to bugprone-header-guard.

[trixirt]

Latest change addresses most of issues.
Outstanding is adding a test when garbage GuardStyle value is fed into config.
The trial testcase used only CHECK-*-NOT which caused complaining from test harness looking for the CHECK- cases.
Looking for something similar in the other checker tests did not turn up anything.
In general it seems like clang-tidy assumes developers get the string names of the options and their values correct and does not provide a lot of (any?) checking.

[aaron.ballman]

In D61508#1509368, @trixirt wrote:

Latest change addresses most of issues.
Outstanding is adding a test when garbage GuardStyle value is fed into config.
The trial testcase used only CHECK-*-NOT which caused complaining from test harness looking for the CHECK- cases.
Looking for something similar in the other checker tests did not turn up anything.
In general it seems like clang-tidy assumes developers get the string names of the options and their values correct and does not provide a lot of (any?) checking.

We're probably quite inconsistent in this regard, and it seems like something we may want to address across the board (not suggesting that you have to fix it, or even as part of this patch). I was envisioning the test as being something like CHECK-MESSAGES-ERROR: 1:1: error: %s is an unsupported header guard style or something along those lines.

clang-tidy/bugprone/HeaderGuardCheck.cpp

31

Can you add a newline for some visual separation?

[trixirt]

Add a newline to separate functions

[trixirt]

clang-tidy/bugprone/HeaderGuardCheck.cpp

31

I did not think ment a empty newline in the param-list, so I assumed you ment the function above and this function could use a new line.

[aaron.ballman]

clang-tidy/bugprone/HeaderGuardCheck.cpp

31

You guessed correctly -- sorry for the lack of clarity. :-)

[garymm]

@aaron.ballman what's remaining for this to be mergeable?

[aaron.ballman]

In D61508#4469426, @garymm wrote:

@aaron.ballman what's remaining for this to be mergeable?

It needs to be rebased onto trunk and it looks like there's some test coverage missing for unsupported styles. It should probably go through more review with the current code owners as well, in case the landscape in clang-tidy has changed in the past few years in this area.

clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "AssertSideEffectCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
 #include "CopyConstructorInitCheck.h"
 #include "DanglingHandleCheck.h"
 #include "ExceptionEscapeCheck.h"
 #include "FoldInitTypeCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"
 #include "ForwardingReferenceOverloadCheck.h"
+#include "HeaderGuardCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundingsCheck.h"
 #include "IntegerDivisionCheck.h"
 #include "LambdaFunctionNameCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisplacedOperatorInStrlenInAllocCheck.h"
 #include "MisplacedWideningCastCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ExceptionEscapeCheck>(
         "bugprone-exception-escape");
     CheckFactories.registerCheck<FoldInitTypeCheck>(
         "bugprone-fold-init-type");
     CheckFactories.registerCheck<ForwardDeclarationNamespaceCheck>(
         "bugprone-forward-declaration-namespace");
     CheckFactories.registerCheck<ForwardingReferenceOverloadCheck>(
         "bugprone-forwarding-reference-overload");
+    CheckFactories.registerCheck<BugproneHeaderGuardCheck>(
+        "bugprone-header-guard");
     CheckFactories.registerCheck<InaccurateEraseCheck>(
         "bugprone-inaccurate-erase");
     CheckFactories.registerCheck<IncorrectRoundingsCheck>(
         "bugprone-incorrect-roundings");
     CheckFactories.registerCheck<IntegerDivisionCheck>(
         "bugprone-integer-division");
     CheckFactories.registerCheck<LambdaFunctionNameCheck>(
         "bugprone-lambda-function-name");

clang-tidy/bugprone/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyBugproneModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   BugproneTidyModule.cpp
   CopyConstructorInitCheck.cpp
   DanglingHandleCheck.cpp
   ExceptionEscapeCheck.cpp
   FoldInitTypeCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
   ForwardingReferenceOverloadCheck.cpp
+  HeaderGuardCheck.cpp
   InaccurateEraseCheck.cpp
   IncorrectRoundingsCheck.cpp
   IntegerDivisionCheck.cpp
   LambdaFunctionNameCheck.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MisplacedOperatorInStrlenInAllocCheck.cpp
   MisplacedWideningCastCheck.cpp

clang-tidy/bugprone/HeaderGuardCheck.h

+//===--- HeaderGuardCheck.h - clang-tidy ------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_HEADERGUARDCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_HEADERGUARDCHECK_H
+
+#include "../utils/HeaderGuard.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Finds and fixes missing header guards and does not enforce any style.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-header-guard.html
+class BugproneHeaderGuardCheck : public utils::HeaderGuardCheck {
+public:
+  BugproneHeaderGuardCheck(StringRef Name, ClangTidyContext *Context);
+  bool shouldSuggestEndifComment(StringRef Filename) override { return false; }
+  std::string getHeaderGuard(StringRef Filename, StringRef OldGuard) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_HEADERGUARDCHECK_H

clang-tidy/bugprone/HeaderGuardCheck.cpp

+//===--- HeaderGuardCheck.cpp - clang-tidy --------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "HeaderGuardCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+BugproneHeaderGuardCheck::BugproneHeaderGuardCheck(StringRef Name,
+                                                   ClangTidyContext *Context)
+    : HeaderGuardCheck(Name, Context) {}
+
+std::string BugproneHeaderGuardCheck::getHeaderGuard(StringRef Filename,
+                                                     StringRef OldGuard) {
+  if (OldGuard.size())
+    return OldGuard;
+
+  std::string Guard = llvm::sys::path::filename(Filename);
+  std::replace(Guard.begin(), Guard.end(), '.', '_');
+  std::replace(Guard.begin(), Guard.end(), '-', '_');
+  return StringRef(Guard).upper();
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 
 - New :doc:`google-readability-avoid-underscore-in-googletest-name
   <clang-tidy/checks/google-readability-avoid-underscore-in-googletest-name>`
   check.
 
   Checks whether there are underscores in googletest test and test case names in
   test macros, which is prohibited by the Googletest FAQ.
 
+- New :doc:`bugprone-header-guard
+  <clang-tidy/checks/bugprone-header-guard>` check.
+
+  Finds and fixes missing header guards and does not enforce any style.
+
 - New :doc:`objc-super-self <clang-tidy/checks/objc-super-self>` check.
 
   Finds invocations of ``-self`` on super instances in initializers of
   subclasses of ``NSObject`` and recommends calling a superclass initializer
   instead.
 
 - New alias :doc:`cppcoreguidelines-explicit-virtual-functions
   <clang-tidy/checks/cppcoreguidelines-explicit-virtual-functions>` to

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

+.. title:: clang-tidy - bugprone-header-guard
+
+bugprone-header-guard
+=====================
+
+Finds and fixes missing header guards and does not enforce any style.
+
+Options
+-------
+
+.. option:: HeaderFileExtensions
+
+   A comma-separated list of filename extensions of header files (the filename
+   extensions should not include "." prefix). Default is `h,hh,hpp,hxx`.
+   For header files without an extension, use an empty string (if there are no
+   other desired extensions) or leave an empty element in the list. e.g.,
+   `h,hh,hpp,hxx,` (note the trailing comma).
+

docs/clang-tidy/checks/google-build-namespaces.rst

 Corresponding cpplint.py check name: `build/namespaces`.
 
 Options
 -------
 
 .. option:: HeaderFileExtensions
 
    A comma-separated list of filename extensions of header files (the filename
-   extensions should not include "." prefix). Default is "h,hh,hpp,hxx".
+   extensions should not include "." prefix). Default is `h,hh,hpp,hxx`.
    For header files without an extension, use an empty string (if there are no
    other desired extensions) or leave an empty element in the list. e.g.,
-   "h,hh,hpp,hxx," (note the trailing comma).
+   `h,hh,hpp,hxx,` (note the trailing comma).

docs/clang-tidy/checks/google-global-names-in-headers.rst

 https://google.github.io/styleguide/cppguide.html#Namespaces.
 
 Options
 -------
 
 .. option:: HeaderFileExtensions
 
    A comma-separated list of filename extensions of header files (the filename
-   extensions should not contain "." prefix). Default is "h".
+   extensions should not include "." prefix). Default is `h,hh,hpp,hxx`.
    For header files without an extension, use an empty string (if there are no
    other desired extensions) or leave an empty element in the list. e.g.,
-   "h,hh,hpp,hxx," (note the trailing comma).
+   `h,hh,hpp,hxx,` (note the trailing comma).

docs/clang-tidy/checks/list.rst

.. toctree::
    bugprone-assert-side-effect
    bugprone-bool-pointer-implicit-conversion
    bugprone-copy-constructor-init
    bugprone-dangling-handle
    bugprone-exception-escape
    bugprone-fold-init-type
    bugprone-forward-declaration-namespace
    bugprone-forwarding-reference-overload
+   bugprone-header-guard
    bugprone-inaccurate-erase
    bugprone-incorrect-roundings
    bugprone-integer-division
    bugprone-lambda-function-name
    bugprone-macro-parentheses
    bugprone-macro-repeated-side-effects
    bugprone-misplaced-operator-in-strlen-in-alloc
    bugprone-misplaced-widening-cast

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

 .. title:: clang-tidy - llvm-header-guard
 
 llvm-header-guard
 =================
 
 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
-   extensions should not include "." prefix). Default is "h,hh,hpp,hxx".
+   extensions should not include "." prefix). Default is `h,hh,hpp,hxx`.
    For header files without an extension, use an empty string (if there are no
    other desired extensions) or leave an empty element in the list. e.g.,
-   "h,hh,hpp,hxx," (note the trailing comma).
+   `h,hh,hpp,hxx,` (note the trailing comma).

docs/clang-tidy/checks/misc-definitions-in-headers.rst

.. code-block:: c++
    constexpr int f10() { return 0; } // OK: constexpr function implies inline.
 
 Options
 -------
 
 .. option:: HeaderFileExtensions
 
    A comma-separated list of filename extensions of header files (the filename
-   extensions should not include "." prefix). Default is "h,hh,hpp,hxx".
+   extensions should not include "." prefix). Default is `h,hh,hpp,hxx`.
    For header files without an extension, use an empty string (if there are no
    other desired extensions) or leave an empty element in the list. e.g.,
-   "h,hh,hpp,hxx," (note the trailing comma).
+   `h,hh,hpp,hxx,` (note the trailing comma).
 
 .. option:: UseHeaderFileExtension
 
    When non-zero, the check will use the file extension to distinguish header
    files. Default is `1`.

test/clang-tidy/bugprone-header-guard.cpp

+// RUN: %check_clang_tidy %s bugprone-header-guard %t -- \
+// RUN:   -config="{CheckOptions: [{key: bugprone-header-guard.HeaderFileExtensions, value: 'cpp'}]}" \
+// RUN:   -header-filter=.* -- 
+
+// CHECK-MESSAGES: 1:1: warning:  header is missing header guard
+
+// CHECK-FIXES: #ifndef BUGPRONE_HEADER_GUARD_CPP_TMP_CPP
+// CHECK-FIXES-NEXT: #define BUGPRONE_HEADER_GUARD_CPP_TMP_CPP
+// CHECK-FIXES: #endif