[clang-tidy] Add some suggestions from code review

Also, renamed from `bugprone-*` to `readability-*`

Author: capitan-davide

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

@@ -33,7 +33,6 @@
 #include "ImplicitWideningOfMultiplicationResultCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncDecInConditionsCheck.h"
-#include "InconsistentIfelseBracesCheck.h"
 #include "IncorrectEnableIfCheck.h"
 #include "IncorrectEnableSharedFromThisCheck.h"
 #include "IncorrectRoundingsCheck.h"
@@ -151,8 +150,6 @@ class BugproneModule : public ClangTidyModule {
         "bugprone-implicit-widening-of-multiplication-result");
     CheckFactories.registerCheck<InaccurateEraseCheck>(
         "bugprone-inaccurate-erase");
-    CheckFactories.registerCheck<InconsistentIfelseBracesCheck>(
-        "bugprone-inconsistent-ifelse-braces");
     CheckFactories.registerCheck<IncorrectEnableIfCheck>(
         "bugprone-incorrect-enable-if");
     CheckFactories.registerCheck<IncorrectEnableSharedFromThisCheck>(

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

@@ -28,7 +28,6 @@ add_clang_library(clangTidyBugproneModule STATIC
   ForwardingReferenceOverloadCheck.cpp
   ImplicitWideningOfMultiplicationResultCheck.cpp
   InaccurateEraseCheck.cpp
-  InconsistentIfelseBracesCheck.cpp
   IncorrectEnableIfCheck.cpp
   IncorrectEnableSharedFromThisCheck.cpp
   InvalidEnumDefaultInitializationCheck.cpp

clang-tools-extra/clang-tidy/readability/CMakeLists.txt

@@ -24,6 +24,7 @@ add_clang_library(clangTidyReadabilityModule STATIC
   IdentifierLengthCheck.cpp
   IdentifierNamingCheck.cpp
   ImplicitBoolConversionCheck.cpp
+  InconsistentIfelseBracesCheck.cpp
   RedundantInlineSpecifierCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
   IsolateDeclarationCheck.cpp

clang-tools-extra/clang-tidy/bugprone/InconsistentIfelseBracesCheck.cpp renamed to clang-tools-extra/clang-tidy/readability/InconsistentIfelseBracesCheck.cpp

@@ -13,7 +13,7 @@
 
 using namespace clang::ast_matchers;
 
-namespace clang::tidy::bugprone {
+namespace clang::tidy::readability {
 
 /// Check that at least one branch of the \p If statement is a \c CompoundStmt.
 static bool shouldHaveBraces(const IfStmt *If) {
@@ -33,11 +33,10 @@ static bool shouldHaveBraces(const IfStmt *If) {
 
 void InconsistentIfelseBracesCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
-      traverse(TK_IgnoreUnlessSpelledInSource,
-               ifStmt(hasElse(anything()),
-                      unless(isConsteval()), // 'if consteval' always has braces
-                      unless(hasParent(ifStmt())))
-                   .bind("if_stmt")),
+      ifStmt(hasElse(anything()),
+             unless(isConsteval()), // 'if consteval' always has braces
+             unless(hasParent(ifStmt())))
+          .bind("if_stmt"),
       this);
 }
 
@@ -50,40 +49,41 @@ void InconsistentIfelseBracesCheck::check(
 }
 
 void InconsistentIfelseBracesCheck::checkIfStmt(
-    const ast_matchers::MatchFinder::MatchResult &Result, const IfStmt *If) {
+    const MatchFinder::MatchResult &Result, const IfStmt *If) {
   const Stmt *Then = If->getThen();
   if (const auto *NestedIf = dyn_cast<const IfStmt>(Then)) {
     // If the then-branch is a nested IfStmt, first we need to add braces to
     // it, then we need to check the inner IfStmt.
-    checkStmt(Result, If->getThen(), If->getRParenLoc(), If->getElseLoc());
+    emitDiagnostic(Result, If->getThen(), If->getRParenLoc(), If->getElseLoc());
     if (shouldHaveBraces(NestedIf))
       checkIfStmt(Result, NestedIf);
   } else if (!isa<CompoundStmt>(Then)) {
-    checkStmt(Result, If->getThen(), If->getRParenLoc(), If->getElseLoc());
+    emitDiagnostic(Result, If->getThen(), If->getRParenLoc(), If->getElseLoc());
   }
 
   if (const Stmt *const Else = If->getElse()) {
     if (const auto *NestedIf = dyn_cast<const IfStmt>(Else))
       checkIfStmt(Result, NestedIf);
     else if (!isa<CompoundStmt>(Else))
-      checkStmt(Result, If->getElse(), If->getElseLoc());
+      emitDiagnostic(Result, If->getElse(), If->getElseLoc());
   }
 }
 
-void InconsistentIfelseBracesCheck::checkStmt(
-    const ast_matchers::MatchFinder::MatchResult &Result, const Stmt *S,
+void InconsistentIfelseBracesCheck::emitDiagnostic(
+    const MatchFinder::MatchResult &Result, const Stmt *S,
     SourceLocation StartLoc, SourceLocation EndLocHint) {
   const SourceManager &SM = *Result.SourceManager;
   const LangOptions &LangOpts = Result.Context->getLangOpts();
 
-  const utils::BraceInsertionHints Hints =
-      utils::getBraceInsertionsHints(S, LangOpts, SM, StartLoc, EndLocHint);
-  if (Hints) {
-    DiagnosticBuilder Diag = diag(Hints.DiagnosticPos, "<message>");
-    if (Hints.offersFixIts()) {
-      Diag << Hints.openingBraceFixIt() << Hints.closingBraceFixIt();
-    }
+  if (!StartLoc.isMacroID()) {
+    const utils::BraceInsertionHints Hints =
+        utils::getBraceInsertionsHints(S, LangOpts, SM, StartLoc, EndLocHint);
+    assert(Hints && Hints.offersFixIts() && "Expected hints or fix-its");
+    diag(Hints.DiagnosticPos, "<message>")
+        << Hints.openingBraceFixIt() << Hints.closingBraceFixIt();
+  } else {
+    diag(StartLoc, "<message-for-macro-expansions>") << StartLoc.isMacroID();
   }
 }
 
-} // namespace clang::tidy::bugprone
+} // namespace clang::tidy::readability

clang-tools-extra/clang-tidy/bugprone/InconsistentIfelseBracesCheck.h renamed to clang-tools-extra/clang-tidy/readability/InconsistentIfelseBracesCheck.h

@@ -6,33 +6,38 @@
 //
 //===----------------------------------------------------------------------===//
 
-#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_INCONSISTENTIFELSEBRACESCHECK_H
-#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_INCONSISTENTIFELSEBRACESCHECK_H
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_INCONSISTENTIFELSEBRACESCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_INCONSISTENTIFELSEBRACESCHECK_H
 
 #include "../ClangTidyCheck.h"
+#include "clang/AST/ASTTypeTraits.h"
+#include <optional>
 
-namespace clang::tidy::bugprone {
+namespace clang::tidy::readability {
 
 /// Detects `if`/`else` statements where one branch uses braces and the other
 /// does not.
 ///
 /// For the user-facing documentation see:
-/// https://clang.llvm.org/extra/clang-tidy/checks/bugprone/inconsistent-ifelse-braces.html
+/// https://clang.llvm.org/extra/clang-tidy/checks/readability/inconsistent-ifelse-braces.html
 class InconsistentIfelseBracesCheck : public ClangTidyCheck {
 public:
   InconsistentIfelseBracesCheck(StringRef Name, ClangTidyContext *Context)
       : ClangTidyCheck(Name, Context) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  std::optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
 
 private:
   void checkIfStmt(const ast_matchers::MatchFinder::MatchResult &Result,
                    const IfStmt *If);
-  void checkStmt(const ast_matchers::MatchFinder::MatchResult &Result,
-                 const Stmt *S, SourceLocation StartLoc,
-                 SourceLocation EndLocHint = {});
+  void emitDiagnostic(const ast_matchers::MatchFinder::MatchResult &Result,
+                      const Stmt *S, SourceLocation StartLoc,
+                      SourceLocation EndLocHint = {});
 };
 
-} // namespace clang::tidy::bugprone
+} // namespace clang::tidy::readability
 
-#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_INCONSISTENTIFELSEBRACESCHECK_H
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_INCONSISTENTIFELSEBRACESCHECK_H

clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp

@@ -30,6 +30,7 @@
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolConversionCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
+#include "InconsistentIfelseBracesCheck.h"
 #include "IsolateDeclarationCheck.h"
 #include "MagicNumbersCheck.h"
 #include "MakeMemberFunctionConstCheck.h"
@@ -110,6 +111,8 @@ class ReadabilityModule : public ClangTidyModule {
         "readability-identifier-naming");
     CheckFactories.registerCheck<ImplicitBoolConversionCheck>(
         "readability-implicit-bool-conversion");
+    CheckFactories.registerCheck<InconsistentIfelseBracesCheck>(
+        "readability-inconsistent-ifelse-braces");
     CheckFactories.registerCheck<MathMissingParenthesesCheck>(
         "readability-math-missing-parentheses");
     CheckFactories.registerCheck<RedundantInlineSpecifierCheck>(

clang-tools-extra/docs/ReleaseNotes.rst

@@ -157,8 +157,8 @@ Improvements to clang-tidy
 New checks
 ^^^^^^^^^^
 
-- New :doc:`bugprone-inconsistent-ifelse-braces
-  <clang-tidy/checks/bugprone/inconsistent-ifelse-braces>` check.
+- New :doc:`readability-inconsistent-ifelse-braces
+  <clang-tidy/checks/readability/inconsistent-ifelse-braces>` check.
 
   Detects ``if``/``else`` statements where one branch uses braces and the other
   does not.

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -101,7 +101,6 @@ Clang-Tidy Checks
    :doc:`bugprone-implicit-widening-of-multiplication-result <bugprone/implicit-widening-of-multiplication-result>`, "Yes"
    :doc:`bugprone-inaccurate-erase <bugprone/inaccurate-erase>`, "Yes"
    :doc:`bugprone-inc-dec-in-conditions <bugprone/inc-dec-in-conditions>`,
-   :doc:`bugprone-inconsistent-ifelse-braces <bugprone/inconsistent-ifelse-braces>`, "Yes"
    :doc:`bugprone-incorrect-enable-if <bugprone/incorrect-enable-if>`, "Yes"
    :doc:`bugprone-incorrect-enable-shared-from-this <bugprone/incorrect-enable-shared-from-this>`, "Yes"
    :doc:`bugprone-incorrect-roundings <bugprone/incorrect-roundings>`,
@@ -387,6 +386,7 @@ Clang-Tidy Checks
    :doc:`readability-identifier-naming <readability/identifier-naming>`, "Yes"
    :doc:`readability-implicit-bool-conversion <readability/implicit-bool-conversion>`, "Yes"
    :doc:`readability-inconsistent-declaration-parameter-name <readability/inconsistent-declaration-parameter-name>`, "Yes"
+   :doc:`readability-inconsistent-ifelse-braces <readability/inconsistent-ifelse-braces>`, "Yes"
    :doc:`readability-isolate-declaration <readability/isolate-declaration>`, "Yes"
    :doc:`readability-magic-numbers <readability/magic-numbers>`,
    :doc:`readability-make-member-function-const <readability/make-member-function-const>`, "Yes"

clang-tools-extra/docs/clang-tidy/checks/bugprone/inconsistent-ifelse-braces.rst renamed to clang-tools-extra/docs/clang-tidy/checks/readability/inconsistent-ifelse-braces.rst

@@ -1,7 +1,7 @@
-.. title:: clang-tidy - bugprone-inconsistent-ifelse-braces
+.. title:: clang-tidy - readability-inconsistent-ifelse-braces
 
-bugprone-inconsistent-ifelse-braces
-===================================
+readability-inconsistent-ifelse-braces
+======================================
 
 Detects ``if``/``else`` statements where one branch uses braces and the other
 does not.

clang-tools-extra/test/clang-tidy/checkers/bugprone/inconsistent-ifelse-braces-consteval-if.cpp renamed to clang-tools-extra/test/clang-tidy/checkers/readability/inconsistent-ifelse-braces-consteval-if.cpp

@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy -std=c++23-or-later %s bugprone-inconsistent-ifelse-braces %t
+// RUN: %check_clang_tidy -std=c++23-or-later %s readability-inconsistent-ifelse-braces %t
 
 bool cond(const char *) { return false; }
 void do_something(const char *) {}
@@ -10,7 +10,7 @@ void f() {
       do_something("if-single-line");
     else {
     }
-    // CHECK-MESSAGES: :[[@LINE-4]]:21: warning: <message> [bugprone-inconsistent-ifelse-braces]
+    // CHECK-MESSAGES: :[[@LINE-4]]:21: warning: <message> [readability-inconsistent-ifelse-braces]
     // CHECK-FIXES: if (cond("if1")) {
     // CHECK-FIXES: } else {
   }
@@ -20,14 +20,14 @@ void f() {
       do_something("if-single-line");
     else {
     }
-    // CHECK-MESSAGES: :[[@LINE-4]]:21: warning: <message> [bugprone-inconsistent-ifelse-braces]
+    // CHECK-MESSAGES: :[[@LINE-4]]:21: warning: <message> [readability-inconsistent-ifelse-braces]
     // CHECK-FIXES: if (cond("if2")) {
     // CHECK-FIXES: } else {
   } else {
     if (cond("if2.1")) {
     } else
       do_something("else-single-line");
-    // CHECK-MESSAGES: :[[@LINE-2]]:11: warning: <message> [bugprone-inconsistent-ifelse-braces]
+    // CHECK-MESSAGES: :[[@LINE-2]]:11: warning: <message> [readability-inconsistent-ifelse-braces]
     // CHECK-FIXES: } else {
     // CHECK-FIXES: }
   }