[clang-tidy] Add modernize-increment-bool check.

[aaron.ballman]

This strikes me as something the compiler should diagnose instead of a clang-tidy check. Incrementing a bool has been deprecated for some time, but it is outright removed in C++17, so I think giving users a migration path as part of the compiler would be far more beneficial. What do you think?

[Prazek]

In D19165#402657, @aaron.ballman wrote:

This strikes me as something the compiler should diagnose instead of a clang-tidy check. Incrementing a bool has been deprecated for some time, but it is outright removed in C++17, so I think giving users a migration path as part of the compiler would be far more beneficial. What do you think?

clang already warns about it, but I don't think it has good fixits.

[Prazek]

Besides comments, looks good to me. But before posting make sure that clang-diagnostics doesn't already have fixits.

clang-tidy/modernize/IncrementBoolCheck.cpp

51

doesn't isInTemplateInstantiation fix it?

clang-tidy/modernize/ModernizeTidyModule.cpp

38

run git-clang-format on your patch, because I see that perhaps this one and other files coud be clang-formatted

docs/clang-tidy/checks/modernize-increment-bool.rst

50–55

put this one under the if statment and remove comments liek this:

if (!first)

second = first++;

// is equivalent to

if (!first) {
    second = false;
    first = true;
  }

[alexfh]

In D19165#403099, @Prazek wrote:

In D19165#402657, @aaron.ballman wrote:

This strikes me as something the compiler should diagnose instead of a clang-tidy check. Incrementing a bool has been deprecated for some time, but it is outright removed in C++17, so I think giving users a migration path as part of the compiler would be far more beneficial. What do you think?

clang already warns about it, but I don't think it has good fixits.

What clang diagnostic flags this issue? I'd rather make sure it provides good fixits than as a clang-tidy check that does almost the same. Clang-tidy can be configured to run Clang diagnostics and it will happily apply fixes if asked to.

[Prazek]

In D19165#403243, @alexfh wrote:

In D19165#403099, @Prazek wrote:

In D19165#402657, @aaron.ballman wrote:

This strikes me as something the compiler should diagnose instead of a clang-tidy check. Incrementing a bool has been deprecated for some time, but it is outright removed in C++17, so I think giving users a migration path as part of the compiler would be far more beneficial. What do you think?

clang already warns about it, but I don't think it has good fixits.

What clang diagnostic flags this issue? I'd rather make sure it provides good fixits than as a clang-tidy check that does almost the same. Clang-tidy can be configured to run Clang diagnostics and it will happily apply fixes if asked to.

-Wdeprecated-increment-bool does it. But what I see from SemaExpr.cpp in CheckIncrementDecrementOperand, it doesn't have any fixits.

[Prazek]

ping @alexfh have you check it?

[mnbvmar]

Clang-formatted code.
Added a simple macro test.
Resolved @Prazek's issues.

[aaron.ballman]

In D19165#419848, @Prazek wrote:

ping @alexfh have you check it?

I would still strongly prefer to see this fixed in CheckIncrementDecrementOperand instead of creating an entire clang-tidy check for it.

[alexfh]

-Wdeprecated-increment-bool does it. But what I see from SemaExpr.cpp in CheckIncrementDecrementOperand, it doesn't have any fixits.

If an automated fix for this issue makes sense at all, it should be implemented in the Clang's diagnostic. BTW, have you seen this pattern in the real code? I haven't. Maybe it's so rare that implementing a fix-it suggestion for it just isn't worth the effort?

clang-tidy/modernize/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyModernizeModule
   DeprecatedHeadersCheck.cpp
+  IncrementBoolCheck.cpp
   LoopConvertCheck.cpp
   LoopConvertUtils.cpp
   MakeUniqueCheck.cpp
   ModernizeTidyModule.cpp
   PassByValueCheck.cpp
   RawStringLiteralCheck.cpp
   RedundantVoidArgCheck.cpp
   ReplaceAutoPtrCheck.cpp

clang-tidy/modernize/IncrementBoolCheck.h

+//===--- IncrementBoolCheck.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_MODERNIZE_INCREMENT_BOOL_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_INCREMENT_BOOL_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// This check replaces deprecated bool incrementation with truth assignment.
+///
+/// Before:
+///  bool x = false, y = ++x;
+/// After:
+///  bool x = false; y = (x = true);
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-increment-bool.html
+class IncrementBoolCheck : public ClangTidyCheck {
+public:
+  IncrementBoolCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_INCREMENT_BOOL_H

clang-tidy/modernize/IncrementBoolCheck.cpp

+//===--- IncrementBoolCheck.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 "IncrementBoolCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include <clang/Lex/Lexer.h>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+static const char ExprName[] = "expr";
+static const char IncrText[] = "++";
+static const char ParentName[] = "parent";
+static const char ParenExprName[] = "parenExpr";
+static const char UnaryOpName[] = "unaryOp";
+
+void IncrementBoolCheck::registerMatchers(MatchFinder *Finder) {
+  const auto ExprMatcher =
+      allOf(has(expr(hasType(booleanType())).bind(ExprName)),
+            hasOperatorName(IncrText));
+
+  const auto ParentMatcher = hasParent(compoundStmt().bind(ParentName));
+  const auto ParMatcher = hasDescendant(parenExpr().bind(ParenExprName));
+
+  // We want to match the operator, no matter if it is a whole statement or not;
+  // no matter if it is a parenthesized expression or not; we just might add
+  // additional bindings.
+  Finder->addMatcher(unaryOperator(ExprMatcher,
+                                   eachOf(ParentMatcher, unless(ParentMatcher)),
+                                   eachOf(ParMatcher, unless(ParMatcher)),
+                                   unless(isInTemplateInstantiation()))
+                         .bind(UnaryOpName),
+                     this);
+}
+
+void IncrementBoolCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<UnaryOperator>(UnaryOpName);
+  const auto *MatchedExpr = Result.Nodes.getNodeAs<Expr>(ExprName);
+
+  // Don't fix if expression type is dependent on template initialization
+  if (MatchedExpr->isTypeDependent())
+    return;
+
+  if (MatchedDecl->getLocStart().isMacroId())
+    return;
+
+  SourceManager &SM = *Result.SourceManager;
+
+  bool Invalid = false;
+  StringRef SubExpr = Lexer::getSourceText(
+      CharSourceRange::getTokenRange(MatchedExpr->getLocStart(),
+                                     MatchedExpr->getLocEnd()),
+      SM, LangOptions(), &Invalid);
+  assert(!Invalid);
+
+
+  bool IsLastOperation = Result.Nodes.getNodeAs<CompoundStmt>(ParentName);
+
+  auto Diag = diag(MatchedDecl->getLocStart(),
+                   "bool incrementation is deprecated, use assignment");
+
+  // Postfix operations are probably impossible to change automatically.
+  if (MatchedDecl->isPostfix() && !IsLastOperation)
+    return;
+
+  bool UseInnerParentheses = Result.Nodes.getNodeAs<ParenExpr>(ParenExprName);
+  bool UseOuterParentheses = !IsLastOperation;
+
+  StringRef Replacement = SubExpr;
+  if (UseInnerParentheses)
+    Replacement = llvm::Twine("(" + Replacement + ")").str();
+
+  Replacement = llvm::Twine(Replacement + " = true").str();
+  if (UseOuterParentheses)
+    Replacement = llvm::Twine("(" + Replacement + ")").str();
+
+  Diag << FixItHint::CreateReplacement(
+      CharSourceRange::getTokenRange(MatchedDecl->getLocStart(),
+                                     MatchedDecl->getLocEnd()),
+      Replacement);
+}
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang

clang-tidy/modernize/ModernizeTidyModule.cpp

 //===--- ModernizeTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "DeprecatedHeadersCheck.h"
+#include "IncrementBoolCheck.h"
 #include "LoopConvertCheck.h"
 #include "MakeUniqueCheck.h"
 #include "PassByValueCheck.h"
 #include "RawStringLiteralCheck.h"
 #include "RedundantVoidArgCheck.h"
 #include "ReplaceAutoPtrCheck.h"
 #include "ShrinkToFitCheck.h"
 #include "UseAutoCheck.h"
 #include "UseDefaultCheck.h"
 #include "UseNullptrCheck.h"
 #include "UseOverrideCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace modernize {
 
 class ModernizeModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<DeprecatedHeadersCheck>(
         "modernize-deprecated-headers");
+    CheckFactories.registerCheck<IncrementBoolCheck>(
+        "modernize-increment-bool");
     CheckFactories.registerCheck<LoopConvertCheck>("modernize-loop-convert");
     CheckFactories.registerCheck<MakeUniqueCheck>("modernize-make-unique");
     CheckFactories.registerCheck<PassByValueCheck>("modernize-pass-by-value");
     CheckFactories.registerCheck<RawStringLiteralCheck>(
         "modernize-raw-string-literal");
     CheckFactories.registerCheck<RedundantVoidArgCheck>(
         "modernize-redundant-void-arg");
     CheckFactories.registerCheck<ReplaceAutoPtrCheck>(

docs/clang-tidy/checks/list.rst

.. toctree::
    misc-throw-by-value-catch-by-reference
    misc-undelegated-constructor
    misc-uniqueptr-reset-release
    misc-unused-alias-decls
    misc-unused-parameters
    misc-unused-raii
    misc-virtual-near-miss
    modernize-deprecated-headers
+   modernize-increment-bool
    modernize-loop-convert
    modernize-make-unique
    modernize-pass-by-value
    modernize-raw-string-literal
    modernize-redundant-void-arg
    modernize-replace-auto-ptr
    modernize-shrink-to-fit
    modernize-use-auto

docs/clang-tidy/checks/modernize-increment-bool.rst

+.. title:: clang-tidy - modernize-increment-bool
+
+modernize-increment-bool
+========================
+
+This check detects the deprecated boolean type incrementation, and wherever possible,
+converts it into truth assignment.
+
+C++17 forbids this behavior. Before C++17 this was equivalent to setting the variable
+to true.
+
+
+Example
+-------
+
+Original:
+
+.. code-block:: c++
+
+  bool variable = false;
+  variable++;
+  ++variable;
+
+  bool another = ++variable;
+  bool third = ++variable && another;
+
+After applying the check:
+
+.. code-block:: c++
+
+  bool variable = false;
+  variable = true;
+  variable = true;  /* Both postfix and prefix incrementations are supported. */
+
+  bool another = (variable = true);
+  bool third = (variable = true) && another;
+
+
+Limitations
+-----------
+
+When postfix boolean incrementation is not the outermost operation done in the instruction,
+tool will not repair the problem (the fix would have to append some instructions after the
+statement). For example:
+
+.. code-block:: c++
+
+  bool first = false, second;
+
+  /* Equivalent to:
+    if (!first) {
+      second = false;
+      first = true;
+    }
+  */
+  if (!first) second = first++;
+

test/clang-tidy/modernize-increment-bool.cpp

+// RUN: %check_clang_tidy %s modernize-increment-bool %t
+
+template<typename T>
+T& f(T &x) { return x; }
+
+template<typename T>
+void g() {
+  T x;
+  x++;
+
+  bool y;
+  y++;
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bool incrementation is deprecated, use assignment
+// CHECK-FIXES: y = true;
+}
+
+int main() {
+  bool b1 = false, b2 = false, b3 = false;
+
+  b1++;
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bool incrementation is deprecated, use assignment
+// CHECK-FIXES: b1 = true;
+
+  bool b4 = f(b1)++;
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: bool incrementation is deprecated, use assignment
+
+  bool b5 = ++f(b1);
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: bool incrementation is deprecated, use assignment
+// CHECK-FIXES: bool b5 = (f(b1) = true);
+
+  (b1 = false)++;
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bool incrementation is deprecated, use assignment
+// CHECK-FIXES: (b1 = false) = true;
+
+  (b1 = b2 = false)++;
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bool incrementation is deprecated, use assignment
+// CHECK-FIXES: (b1 = b2 = false) = true;
+
+  ++(b1 = b2 = false);
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bool incrementation is deprecated, use assignment
+// CHECK-FIXES: (b1 = b2 = false) = true;
+
+  b3 = b1++ && b2;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: bool incrementation is deprecated, use assignment
+
+  b3 = ++(b1 |= b4) && b3;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: bool incrementation is deprecated, use assignment
+// CHECK-FIXES: b3 = ((b1 |= b4) = true) && b3;
+
+  int x = 8;
+  x++;
+
+  g<int>();
+  g<bool>();
+}