[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

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

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

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

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

48–55

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

48–55

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

[aaron.ballman]

clang-tidy/modernize/UseBoolLiteralsCheck.cpp

48–55

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

48–55

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

48–55

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

48–55

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

48–55

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

[omtcyfz]

clang-tidy/modernize/UseBoolLiteralsCheck.cpp

48–55

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

48–55

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

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

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

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

[alexfh]

clang-tidy/modernize/UseBoolLiteralsCheck.cpp

39

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

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-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()))),
+          anyOf(hasTrueExpression(ignoringParenImpCasts(
+                    integerLiteral().bind("trueBranchLiteral"))),
+                anything()),
+          anyOf(hasFalseExpression(ignoringParenImpCasts(
+                    integerLiteral().bind("falseBranchLiteral"))),
+                anything())),
+      this);
 }
 
 void UseBoolLiteralsCheck::check(const MatchFinder::MatchResult &Result) {
-  const auto *Literal = Result.Nodes.getNodeAs<IntegerLiteral>("literal");
+  for (const auto &BindingName :
+       {"literal", "trueBranchLiteral", "falseBranchLiteral"}) {
+    const auto *Literal = Result.Nodes.getNodeAs<IntegerLiteral>(BindingName);
+    if (!Literal)
+      continue;
     const auto *Cast = Result.Nodes.getNodeAs<Expr>("cast");
     bool LiteralBooleanValue = Literal->getValue().getBoolValue();
 
     if (Literal->isInstantiationDependent())
-    return;
+      continue;
 
     const Expr *Expression = Cast ? Cast : Literal;
 
     auto Diag =
         diag(Expression->getExprLoc(),
              "converting integer literal to bool, use bool literal instead");
 
     if (!Expression->getLocStart().isMacroID())
       Diag << FixItHint::CreateReplacement(
           Expression->getSourceRange(), LiteralBooleanValue ? "true" : "false");
   }
+}
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

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

int main() {
   valueDependentTemplateFunction<1>();
 
   anotherTemplateFunction(1);
 
   IntToTrue = 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: {{.*}}
   // CHECK-FIXES: {{^ *}}IntToTrue = true;{{$}}
 }
+
+static int Value = 1;
+
+bool Function1() {
+  bool Result = Value == 1 ? 1 : 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:30: {{.*}}
+  // CHECK-MESSAGES: :[[@LINE-2]]:34: {{.*}}
+  // CHECK-FIXES: {{^ *}}bool Result = Value == 1 ? true : false;{{$}}
+  return Result;
+}
+
+bool Function2() {
+  return Value == 1 ? 1 : 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: {{.*}}
+  // CHECK-MESSAGES: :[[@LINE-2]]:27: {{.*}}
+  // CHECK-FIXES: {{^ *}}return Value == 1 ? true : false;{{$}}
+}