[clang-tidy] Adds modernize-use-bool-literals check.

[staronj]

Summary

Adds new check to clang-tidy.

Check finds conversions from integer literal to bool.

[Prazek]

Add note in docs/Releasenotes.rst. Besides that it looks good to me, but you still needs the acceptance of Alex (alexfh), who is clang-tidy code owner,

[staronj]

Adds note in docs/Releasenotes.rst.

[Eugene.Zelenko]

Isn't readability-implicit-bool-cast¶ should catch such issues? If not, I think will be good idea to improve that check instead of introducing new one.

[alexfh]

1. Looks like modernize would be a better fit for this check, since it targets a pattern common for pre-standard C++ (or C code).
2. Please clang-format the code.
3. Please run the check on a large enough codebase (at least, on the whole LLVM) to ensure it doesn't crash and doesn't produce obvious false positives.

[Prazek]

In D18745#390739, @Eugene.Zelenko wrote:

Isn't readability-implicit-bool-cast¶ should catch such issues? If not, I think will be good idea to improve that check instead of introducing new one.

I wouldn't add this functionality there. I see that readability-implicit-bool-cast aims much different problem.

[staronj]

1. Name changed from misc-use-bool-literals to modernize-use-bool-literals.
2. Code clang-formatted.
3. Check ran on LLVM code.

[Prazek]

So the testing on llvm shows mostly one case - using DEBUG macro like this:

/home/prazek/llvm/lib/Support/APInt.cpp:1656:9: warning: implicitly converting integer literal to bool inside macro, use bool literal instead [modernize-use-bool-literals]

DEBUG(dbgs() << " " << r[i]);
^

/home/prazek/llvm/include/llvm/Support/Debug.h:92:18: note: expanded from macro 'DEBUG'
#define DEBUG(X) DEBUG_WITH_TYPE(DEBUG_TYPE, X)

^

/home/prazek/llvm/include/llvm/Support/Debug.h:69:48: note: expanded from macro 'DEBUG_WITH_TYPE'
#define DEBUG_WITH_TYPE(TYPE, X) do { } while (0)

     ^

Some programers maybe would like to supress this in the case of expressions like while(1), or specially when it is inside macro.
What do you think guys? Should we add some special option like supress-for-while or supress-macro-while?

[LegalizeAdulthood]

docs/clang-tidy/checks/modernize-use-bool-literals.rst

19

Please ensure the file ends with a newline.

test/clang-tidy/modernize-use-bool-literals.cpp

4

There is more initialization syntax than just assignment. Here are the ones I can think of:

What happens when I write bool foo{1}; or bool foo(1);?

What happens when I write:

class foo {
public:
  foo() : b(0) {}
  foo(int) : b{0} {}
private:
  bool b;
  bool c{0};
  bool d(0);
  bool e = 0;
};

Should we warn an a bool is initialized by a call to a constexpr function that returns an integral type?

[Eugene.Zelenko]

In D18745#391210, @Prazek wrote:

In D18745#390739, @Eugene.Zelenko wrote:

Isn't readability-implicit-bool-cast¶ should catch such issues? If not, I think will be good idea to improve that check instead of introducing new one.

I wouldn't add this functionality there. I see that readability-implicit-bool-cast aims much different problem.

We have a lot of casts related checks, so may be will be good idea to introduce dedicated group for such them?

[krystyna]

lgtm

[mnbvmar]

You could also think whether char literals should be converted to true/false, too. Apart from this (and other people's doubts), everything's fine.

[staronj]

1. Adds newline at the end of modernize-use-bool-literals.rst file.
2. Change names in check test for better readability.
3. Adds some new test cases.

[alexfh]

clang-tidy/modernize/UseBoolLiteralsCheck.cpp

40

Use !x.empty() instead of x.size() >= 1.

clang-tidy/modernize/UseBoolLiteralsCheck.h

20

What about the ones explicitly cast to bool? Do we want to warn on them?

test/clang-tidy/modernize-use-bool-literals.cpp

19

Please add CHECK-FIXES to ensure that both the definition of the macro and the code using it are not changed.

[staronj]

Check now finds implicit and explicit conversions from integer literal to bool.

Added tests with templates and macros.
Changed function isPreprocessorDependent to use isMacroId function.

[alexfh]

clang-tidy/modernize/UseBoolLiteralsCheck.cpp

30

Adding two matchers that do almost the same checks is wasteful. You can do the same in a single matcher, which will do twice less work:

implicitCastExpr(has(integerLiteral().bind("literal")),
                 hasImplicitDestinationType(qualType(booleanType())),
                 unless(isInTemplateInstantiation()),
                 hasParent(anyOf(explicitCastExpr().bind("cast"), anything())))

(if anything() here doesn't work, you can probably use expr() or something else).

56

Can you explain, why is it important to note that this happens "inside a macro"?

58

I think, you can write if (Cast) Literal = Cast; and the rest of the code will be much shorter. Also, the isPreprocessorDependent doesn't seem to be needed.

[staronj]

clang-tidy/modernize/UseBoolLiteralsCheck.cpp

56

"Inside a macro" removed.

[alexfh]

Looks good!

[Prazek]

gg for your first check :)

clang-tidy/modernize/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyModernizeModule
   DeprecatedHeadersCheck.cpp
   LoopConvertCheck.cpp
   LoopConvertUtils.cpp
   MakeUniqueCheck.cpp
   ModernizeTidyModule.cpp
   PassByValueCheck.cpp
   RawStringLiteralCheck.cpp
   RedundantVoidArgCheck.cpp
   ReplaceAutoPtrCheck.cpp
   ShrinkToFitCheck.cpp
   UseAutoCheck.cpp
+  UseBoolLiteralsCheck.cpp
   UseDefaultCheck.cpp
   UseNullptrCheck.cpp
   UseOverrideCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyReadabilityModule
   clangTidyUtils
   )

clang-tidy/modernize/ModernizeTidyModule.cpp

 #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 "UseBoolLiteralsCheck.h"
 #include "UseDefaultCheck.h"
 #include "UseNullptrCheck.h"
 #include "UseOverrideCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<RawStringLiteralCheck>(
         "modernize-raw-string-literal");
     CheckFactories.registerCheck<RedundantVoidArgCheck>(
         "modernize-redundant-void-arg");
     CheckFactories.registerCheck<ReplaceAutoPtrCheck>(
         "modernize-replace-auto-ptr");
     CheckFactories.registerCheck<ShrinkToFitCheck>("modernize-shrink-to-fit");
     CheckFactories.registerCheck<UseAutoCheck>("modernize-use-auto");
+    CheckFactories.registerCheck<UseBoolLiteralsCheck>(
+        "modernize-use-bool-literals");
     CheckFactories.registerCheck<UseDefaultCheck>("modernize-use-default");
     CheckFactories.registerCheck<UseNullptrCheck>("modernize-use-nullptr");
     CheckFactories.registerCheck<UseOverrideCheck>("modernize-use-override");
   }
 
   ClangTidyOptions getModuleOptions() override {
     ClangTidyOptions Options;
     auto &Opts = Options.CheckOptions;

clang-tidy/modernize/UseBoolLiteralsCheck.h

+//===--- UseBoolLiteralsCheck.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_USE_BOOL_LITERALS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_BOOL_LITERALS_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// Finds integer literals which are cast to bool.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-bool-literals.html
+class UseBoolLiteralsCheck : public ClangTidyCheck {
+public:
+  UseBoolLiteralsCheck(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_USE_BOOL_LITERALS_H

clang-tidy/modernize/UseBoolLiteralsCheck.cpp

+//===--- UseBoolLiteralsCheck.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 "UseBoolLiteralsCheck.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 {
+
+void UseBoolLiteralsCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  auto hasIntegerLiteralCastToBool =
+      allOf(has(integerLiteral().bind("literal")),
+            hasImplicitDestinationType(qualType(booleanType())),
+            unless(isInTemplateInstantiation()));
+
+  Finder->addMatcher(implicitCastExpr(hasIntegerLiteralCastToBool,
+                                      unless(hasParent(explicitCastExpr()))),
+                     this);
+
+  Finder->addMatcher(
+      implicitCastExpr(hasIntegerLiteralCastToBool,
+                       hasParent(explicitCastExpr().bind("cast"))),
+      this);
+}
+
+/// \brief Checks, if  \p Statement is preprocessor dependent.
+/// If \p Statement is nullptr returns false.
+static bool isPreprocessorDependent(const Stmt *Statement) {
+  return Statement != nullptr && Statement->getLocStart().isMacroID();
+}
+
+void UseBoolLiteralsCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Literal = Result.Nodes.getNodeAs<IntegerLiteral>("literal");
+  const auto *Cast = Result.Nodes.getNodeAs<ExplicitCastExpr>("cast");
+  bool LiteralBooleanValue = Literal->getValue().getBoolValue();
+
+  if (Literal->isInstantiationDependent())
+    return;
+
+  auto Diag = diag(Literal->getLocation(),
+                   "converting integer literal to "
+                   "bool%select{| inside a macro}0, use bool literal instead");
+
+  if (isPreprocessorDependent(Literal) || isPreprocessorDependent(Cast)) {
+    Diag << 1;
+  } else if (Cast != nullptr) {
+    Diag << 0
+         << FixItHint::CreateReplacement(
+                Cast->getSourceRange(), LiteralBooleanValue ? "true" : "false");
+  } else {
+    Diag << 0 << FixItHint::CreateReplacement(Literal->getSourceRange(),
+                                              LiteralBooleanValue ? "true"
+                                                                  : "false");
+  }
+}
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

- New `modernize-deprecated-headers
   Replaces C standard library headers with their C++ alternatives.
 
 - New `modernize-raw-string-literal
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-raw-string-literal.html>`_ check
 
   Selectively replaces string literals containing escaped characters with raw
   string literals.
 
+- New `modernize-use-bool-literals
+  <http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-bool-literals.html>`_ check
+
+  Finds integer literals which are cast to bool.
+
 - New `performance-faster-string-find
   <http://clang.llvm.org/extra/clang-tidy/checks/performance-faster-string-find.html>`_ check
 
   Optimize calls to ``std::string::find()`` and friends when the needle passed
   is a single character string literal.
 
 - New `performance-implicit-cast-in-loop
   <http://clang.llvm.org/extra/clang-tidy/checks/performance-implicit-cast-in-loop.html>`_ check

docs/clang-tidy/checks/list.rst

.. toctree::
    cppcoreguidelines-pro-type-static-cast-downcast
    cppcoreguidelines-pro-type-union-access
    cppcoreguidelines-pro-type-vararg
    google-build-explicit-make-pair
    google-build-namespaces
    google-build-using-namespace
    google-explicit-constructor
    google-global-names-in-headers
-   google-readability-braces-around-statements (redirects to readability-braces-around-statements) <readability-braces-around-statements>
+   google-readability-braces-around-statements (redirects to readability-braces-around-statements) <google-readability-braces-around-statements>
    google-readability-casting
-   google-readability-function-size (redirects to readability-function-size) <readability-function-size>
+   google-readability-function-size (redirects to readability-function-size) <google-readability-function-size>
    google-readability-namespace-comments
    google-readability-redundant-smartptr-get
    google-readability-todo
    google-runtime-int
    google-runtime-member-string-references
    google-runtime-memset
    google-runtime-operator
    google-runtime-references
.. toctree::
    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
+   modernize-use-bool-literals
    modernize-use-default
    modernize-use-nullptr
    modernize-use-override
    performance-faster-string-find
    performance-for-range-copy
    performance-implicit-cast-in-loop
    performance-unnecessary-copy-initialization
    performance-unnecessary-value-param

docs/clang-tidy/checks/modernize-use-bool-literals.rst

+.. title:: clang-tidy - modernize-use-bool-literals
+
+modernize-use-bool-literals
+===========================
+
+Finds integer literals which are cast to bool.
+
+.. code-block:: c++
+
+  bool p = 1;
+  bool f = static_cast<bool>(1);
+  std::ios_base::sync_with_stdio(0);
+
+  // transforms to
+
+  bool p = true;
+  bool f = true;
+  std::ios_base::sync_with_stdio(false);

test/clang-tidy/modernize-use-bool-literals.cpp

+// RUN: %check_clang_tidy %s modernize-use-bool-literals %t
+
+bool IntToTrue = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: converting integer literal to bool, use bool literal instead [modernize-use-bool-literals]
+// CHECK-FIXES: {{^}}bool IntToTrue = true;{{$}}
+
+bool IntToFalse(0);
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: {{.*}}
+// CHECK-FIXES: {{^}}bool IntToFalse(false);{{$}}
+
+bool LongLongToTrue{0x1LL};
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: {{.*}}
+// CHECK-FIXES: {{^}}bool LongLongToTrue{true};{{$}}
+
+bool ExplicitCStyleIntToFalse = (bool)0;
+// CHECK-MESSAGES: :[[@LINE-1]]:39: warning: {{.*}}
+// CHECK-FIXES: {{^}}bool ExplicitCStyleIntToFalse = false;{{$}}
+
+bool ExplicitFunctionalIntToFalse = bool(0);
+// CHECK-MESSAGES: :[[@LINE-1]]:42: warning: {{.*}}
+// CHECK-FIXES: {{^}}bool ExplicitFunctionalIntToFalse = false;{{$}}
+
+bool ExplicitStaticIntToFalse = static_cast<bool>(0);
+// CHECK-MESSAGES: :[[@LINE-1]]:51: warning: {{.*}}
+// CHECK-FIXES: {{^}}bool ExplicitStaticIntToFalse = false;{{$}}
+
+#define TRUE_MACRO 1
+// CHECK-FIXES: {{^}}#define TRUE_MACRO 1{{$}}
+
+bool MacroIntToTrue = TRUE_MACRO;
+// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: converting integer literal to bool inside a macro, use bool literal instead [modernize-use-bool-literals]
+// CHECK-FIXES: {{^}}bool MacroIntToTrue = TRUE_MACRO;{{$}}
+
+#define FALSE_MACRO bool(0)
+// CHECK-FIXES: {{^}}#define FALSE_MACRO bool(0){{$}}
+
+
+bool TrueBool = true; // OK
+
+bool FalseBool = FALSE_MACRO;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: {{.*}}
+// CHECK-FIXES: {{^}}bool FalseBool = FALSE_MACRO;{{$}}
+
+void boolFunction(bool bar) {
+
+}
+
+char Character = 0; // OK
+
+unsigned long long LongInteger = 1; // OK
+
+#define MACRO_DEPENDENT_CAST(x) static_cast<bool>(x)
+// CHECK-FIXES: {{^}}#define MACRO_DEPENDENT_CAST(x) static_cast<bool>(x){{$}}
+
+bool MacroDependentBool = MACRO_DEPENDENT_CAST(0);
+// CHECK-MESSAGES: :[[@LINE-1]]:48: warning: {{.*}}
+// CHECK-FIXES: {{^}}bool MacroDependentBool = MACRO_DEPENDENT_CAST(0);{{$}}
+
+class FooClass {
+  public:
+  FooClass() : JustBool(0) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: {{.*}}
+  // CHECK-FIXES: {{^ *}}FooClass() : JustBool(false) {}{{$}}
+  FooClass(int) : JustBool{0} {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: {{.*}}
+  // CHECK-FIXES: {{^ *}}FooClass(int) : JustBool{false} {}{{$}}
+  private:
+  bool JustBool;
+  bool BoolWithBraces{0};
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: {{.*}}
+  // CHECK-FIXES: {{^ *}}bool BoolWithBraces{false};{{$}}
+  bool BoolFromInt = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: {{.*}}
+  // CHECK-FIXES: {{^ *}}bool BoolFromInt = false;{{$}}
+  bool SimpleBool = true; // OK
+};
+
+template<typename type>
+void templateFunction(type) {
+  type TemplateType = 0;
+  // CHECK-FIXES: {{^ *}}type TemplateType = 0;{{$}}
+  return;
+}
+
+template<int c>
+void valueDependentTemplateFunction() {
+  bool Boolean = c;
+  // CHECK-FIXES: {{^ *}}bool Boolean = c;{{$}}
+  return;
+}
+
+template<typename type>
+void anotherTemplateFunction(type) {
+  bool JustBool = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: {{.*}}
+  // CHECK-FIXES: {{^ *}}bool JustBool = false;{{$}}
+  return;
+}
+
+int main() {
+  boolFunction(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: {{.*}}
+  // CHECK-FIXES: {{^ *}}boolFunction(true);{{$}}
+
+  boolFunction(false);
+
+  templateFunction(0);
+
+  templateFunction(false);
+
+  valueDependentTemplateFunction<1>();
+
+  anotherTemplateFunction(1);
+
+  IntToTrue = 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}}
+  // CHECK-FIXES: {{^ *}}IntToTrue = true;{{$}}
+}