[clang-tidy] enhance modernize-use-bool-literals to check ternary operator

[omtcyfz]

Summary

Fixing bug https://llvm.org/bugs/show_bug.cgi?id=28854.

Implicit cast with ternary operator makes AST look in a way in which it feels reasonable to add second matcher to detect implicit casts in ternary operators.

[Prazek]

clang-tidy/modernize/UseBoolLiteralsCheck.cpp

38–43 ↗

(On Diff #67089)

Do I understand it correctly, that you have to have this 2 anyOf matchers, because if you would have it in one then because anyOf is lazy, it would not bind to the second branch?

I think it would be good to write some comment about it, because on the first sight it looks like it could be simplified.

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

119 ↗

(On Diff #67089)

Please add a test with only one branch with constant, and none of them.
Also one test like
bool Result = Value == 1 ? true : 0;
or
bool Result = Value == 1 ? false : bool(0);

[omtcyfz]

Comment anyOf() part in check's matcher and extend testset.

[omtcyfz]

clang-tidy/modernize/UseBoolLiteralsCheck.cpp

38–43 ↗

(On Diff #67089)

Do I understand it correctly, that you have to have this 2 anyOf matchers, because if you would have it in one then because anyOf is lazy, it would not bind to the second branch?

Yes, exactly. In order to bind both I do anyOf().

I think it would be good to write some comment about it, because on the first sight it looks like it could be simplified.

Done.

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

119 ↗

(On Diff #67089)

Added a bunch of new tests.

[Prazek]

LGTM, but wait a day, maybe someone will have other comments.

[omtcyfz]

Thanks.

Yes, sure.

[aaron.ballman]

Would it also make sense to support BinaryConditionalOperator as well as the ternary operator?

clang-tidy/modernize/UseBoolLiteralsCheck.cpp

51–52 ↗

(On Diff #67095)

Any reason not to name the bind "literal" in all three cases? That eliminates the need for the loop entirely, since check() will trigger for each instance of a match.

[omtcyfz]

clang-tidy/modernize/UseBoolLiteralsCheck.cpp

51–52 ↗

(On Diff #67095)

It doesn't make sense to try binding both TrueExpression and FalseExpression literals to a single value.

[aaron.ballman]

clang-tidy/modernize/UseBoolLiteralsCheck.cpp

51–52 ↗

(On Diff #67095)

Why? In all three cases, you don't care what matched, just that *some* case is matched. None of the logic in check() relies on which part of the expression is matched.

[omtcyfz]

clang-tidy/modernize/UseBoolLiteralsCheck.cpp

51–52 ↗

(On Diff #67095)

Well, in case of second matcher I may have two literals matched upon triggering. I don't understand how I could possibly get two literals bound to one value after one matcher got triggered.

Am I missing something?

[aaron.ballman]

clang-tidy/modernize/UseBoolLiteralsCheck.cpp

51–52 ↗

(On Diff #67095)

One matcher isn't what's getting triggered then, is it? I could be wrong on this point, but I thought that in that case, check() would be called twice, once for each literal. Is that not the case?

[omtcyfz]

clang-tidy/modernize/UseBoolLiteralsCheck.cpp

51–52 ↗

(On Diff #67095)

From what I understand about how matchers work, it is not. Plus, I checked (just named everything "literal" and removed for (const auto ... just to double check in case I was wrong.

[aaron.ballman]

clang-tidy/modernize/UseBoolLiteralsCheck.cpp

51–52 ↗

(On Diff #67095)

Thank you for checking -- I learned something new! :-)

[omtcyfz]

clang-tidy/modernize/UseBoolLiteralsCheck.cpp

51–52 ↗

(On Diff #67095)

No problem :)

One case in which your solution would work is splitting second matcher into two: one with hasTrueExpression and one with hasFalseExpression, then binding to "literal" would work. But it'll be inefficient and three names of bindings don't seem like too much hardcoding after all.

[aaron.ballman]

LGTM! We may want to consider BinaryConditionalOperator as well, but that can be in as a separate patch.

clang-tidy/modernize/UseBoolLiteralsCheck.cpp

51–52 ↗

(On Diff #67095)

I agree, that would be a step backwards.

[omtcyfz]

Added ternary operator example to docs.

[omtcyfz]

In D23243#508097, @aaron.ballman wrote:

LGTM! We may want to consider BinaryConditionalOperator as well, but that can be in as a separate patch.

Well, I personally have never seen BinaryConditionalOperator in my life.

And honestly I'm not a fan of supporting GNU extensions - what makes sense to me is to use BinaryConditionalOperator in a check which suggests not using GNU extensions.

Anyway, I'd be glad if @alexfh could jump in and take a look.

[Prazek]

Yea, I also have never heard of it. I don't think it is worth even discussing

[alexfh]

clang-tidy/modernize/UseBoolLiteralsCheck.cpp

41 ↗

(On Diff #67102)

I think, Aaron was talking about eachOf matcher that will actually generate a separate match for each of its submatchers.

[aaron.ballman]

clang-tidy/modernize/UseBoolLiteralsCheck.cpp

41 ↗

(On Diff #67102)

I *knew* we had something that would do this! Thank you for the reminder, Alex. :-)

[omtcyfz]

Use shiny eachOf and bind everything to "literal" value.

clang-tidy/modernize/UseBoolLiteralsCheck.cpp

41 ↗

(On Diff #67102)

Yup. Seems like I learned about something, too :)

[alexfh]

clang-tidy/modernize/UseBoolLiteralsCheck.cpp

39 ↗

(On Diff #67169)

Remove anyOf and anything. And the comment.

[omtcyfz]

Remove redundant anyOf and anything.

[alexfh]

LG with a nit.

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

124 ↗

(On Diff #67189)

The check lines should be a bit stricter. Please change to :[[@LINE-1]]:30: warning: converting integer literal to bool. Other new patterns in this file as well.

[omtcyfz]

More strict test messages.

clang-tools-extra/trunk/clang-tidy/modernize/UseBoolLiteralsCheck.cpp

void UseBoolLiteralsCheck::registerMatchers(MatchFinder *Finder) {
 
   Finder->addMatcher(
       implicitCastExpr(
           has(ignoringParenImpCasts(integerLiteral().bind("literal"))),
           hasImplicitDestinationType(qualType(booleanType())),
           unless(isInTemplateInstantiation()),
           anyOf(hasParent(explicitCastExpr().bind("cast")), anything())),
       this);
+
+  Finder->addMatcher(
+      conditionalOperator(
+          hasParent(implicitCastExpr(
+              hasImplicitDestinationType(qualType(booleanType())),
+              unless(isInTemplateInstantiation()))),
+          eachOf(hasTrueExpression(
+                     ignoringParenImpCasts(integerLiteral().bind("literal"))),
+                 hasFalseExpression(
+                     ignoringParenImpCasts(integerLiteral().bind("literal"))))),
+      this);
 }
 
 void UseBoolLiteralsCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Literal = Result.Nodes.getNodeAs<IntegerLiteral>("literal");
   const auto *Cast = Result.Nodes.getNodeAs<Expr>("cast");
   bool LiteralBooleanValue = Literal->getValue().getBoolValue();
 
   if (Literal->isInstantiationDependent())

clang-tools-extra/trunk/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);
+  bool x = p ? 1 : 0;
 
   // transforms to
 
   bool p = true;
   bool f = true;
   std::ios_base::sync_with_stdio(false);
+  bool x = p ? true : false;

clang-tools-extra/trunk/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-MESSAGES: :[[@LINE-1]]:17: warning: converting integer literal to bool
 // CHECK-FIXES: {{^}}bool IntToFalse(false);{{$}}
 
 bool LongLongToTrue{0x1LL};
-// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: {{.*}}
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: converting integer literal to bool
 // CHECK-FIXES: {{^}}bool LongLongToTrue{true};{{$}}
 
 bool ExplicitCStyleIntToFalse = (bool)0;
-// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: {{.*}}
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: converting integer literal to bool
 // CHECK-FIXES: {{^}}bool ExplicitCStyleIntToFalse = false;{{$}}
 
 bool ExplicitFunctionalIntToFalse = bool(0);
-// CHECK-MESSAGES: :[[@LINE-1]]:37: warning: {{.*}}
+// CHECK-MESSAGES: :[[@LINE-1]]:37: warning: converting integer literal to bool
 // CHECK-FIXES: {{^}}bool ExplicitFunctionalIntToFalse = false;{{$}}
 
 bool ExplicitStaticIntToFalse = static_cast<bool>(0);
-// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: {{.*}}
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: converting integer literal to bool
 // 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, use bool literal instead [modernize-use-bool-literals]
+// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: converting integer literal to bool
 // CHECK-FIXES: {{^}}bool MacroIntToTrue = TRUE_MACRO;{{$}}
 
 #define FALSE_MACRO bool(0)
 // CHECK-FIXES: {{^}}#define FALSE_MACRO bool(0){{$}}
 
 bool TrueBool = true; // OK
 
 bool FalseBool = bool(FALSE_MACRO);
-// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: {{.*}}
+// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: converting integer literal to bool
 // CHECK-FIXES: {{^}}bool FalseBool = bool(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]]:27: warning: {{.*}}
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: converting integer literal to bool
 // CHECK-FIXES: {{^}}bool MacroDependentBool = MACRO_DEPENDENT_CAST(0);{{$}}
 
 bool ManyMacrosDependent = MACRO_DEPENDENT_CAST(FALSE_MACRO);
-// CHECK-MESSAGES: :[[@LINE-1]]:49: warning: {{.*}}
+// CHECK-MESSAGES: :[[@LINE-1]]:49: warning: converting integer literal to bool
 // CHECK-FIXES: {{^}}bool ManyMacrosDependent = MACRO_DEPENDENT_CAST(FALSE_MACRO);{{$}}
 
 class FooClass {
   public:
   FooClass() : JustBool(0) {}
-  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: {{.*}}
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: converting integer literal to bool
   // CHECK-FIXES: {{^ *}}FooClass() : JustBool(false) {}{{$}}
   FooClass(int) : JustBool{0} {}
-  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: {{.*}}
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: converting integer literal to bool
   // CHECK-FIXES: {{^ *}}FooClass(int) : JustBool{false} {}{{$}}
   private:
   bool JustBool;
   bool BoolWithBraces{0};
-  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: {{.*}}
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: converting integer literal to bool
   // CHECK-FIXES: {{^ *}}bool BoolWithBraces{false};{{$}}
   bool BoolFromInt = 0;
-  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: {{.*}}
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: converting integer literal to bool
   // CHECK-FIXES: {{^ *}}bool BoolFromInt = false;{{$}}
   bool SimpleBool = true; // OK
 };
 
 template<typename type>
 void templateFunction(type) {
   type TemplateType = 0;
   // CHECK-FIXES: {{^ *}}type TemplateType = 0;{{$}}
 }
 
 template<int c>
 void valueDependentTemplateFunction() {
   bool Boolean = c;
   // CHECK-FIXES: {{^ *}}bool Boolean = c;{{$}}
 }
 
 template<typename type>
 void anotherTemplateFunction(type) {
   bool JustBool = 0;
-  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: {{.*}}
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: converting integer literal to bool
   // CHECK-FIXES: {{^ *}}bool JustBool = false;{{$}}
 }
 
 int main() {
   boolFunction(1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: {{.*}}
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: converting integer literal to bool
   // CHECK-FIXES: {{^ *}}boolFunction(true);{{$}}
 
   boolFunction(false);
 
   templateFunction(0);
 
   templateFunction(false);
 
   valueDependentTemplateFunction<1>();
 
   anotherTemplateFunction(1);
 
   IntToTrue = 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}}
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: converting integer literal to bool
   // CHECK-FIXES: {{^ *}}IntToTrue = true;{{$}}
 }
+
+static int Value = 1;
+
+bool Function1() {
+  bool Result = Value == 1 ? 1 : 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: converting integer literal to bool
+  // CHECK-MESSAGES: :[[@LINE-2]]:34: warning: converting integer literal to bool
+  // CHECK-FIXES: {{^ *}}bool Result = Value == 1 ? true : false;{{$}}
+  return Result;
+}
+
+bool Function2() {
+  return Value == 1 ? 1 : 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: converting integer literal to bool
+  // CHECK-MESSAGES: :[[@LINE-2]]:27: warning: converting integer literal to bool
+  // CHECK-FIXES: {{^ *}}return Value == 1 ? true : false;{{$}}
+}
+
+void foo() {
+  bool Result;
+  Result = Value == 1 ? true : 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: converting integer literal to bool
+  // CHECK-FIXES: {{^ *}}Result = Value == 1 ? true : false;{{$}}
+  Result = Value == 1 ? false : bool(0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: converting integer literal to bool
+  // CHECK-FIXES: {{^ *}}Result = Value == 1 ? false : false;{{$}}
+  Result = Value == 1 ? (bool)0 : false;
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: converting integer literal to bool
+  // CHECK-FIXES: {{^ *}}Result = Value == 1 ? false : false;{{$}}
+}