[clang-tidy] FIXIT for implicit bool conversion now obeys upper case suffixes if enforced.

[pnappa]

Summary

Fix for https://bugs.llvm.org/show_bug.cgi?id=41199#c1

Previously, if a user implicitly cast to a bool (say, in a conditional statement), the fix would be to add an explicit comparison. So, for a floating point implicit case to bool, from if (f), the synthesised code would be if (f != 0.0f).

Even if the flag "readability-uppercase-literal-suffix" was enabled, the synthesised suffix would be lowercase. This commit changes that, such that if that flag is enabled when "readability-implicit-bool-conversion" is enabled, the synthesised suffix is uppercase.

A non-covered case is when "modernize-use-default-member-init" is enabled, lower-case suffixes may still be synthesised (I think, based off the code). Any RFC whether this should be made a different issue or whether or not this behaviour should be added in?

Intended as my first commit to the llvm-project.

[lebedev.ri]

Please always upload all patches with full context (-U99999)
I'm not sure why we want this? What is wrong with simply applying clang-tidy twice?

[aaron.ballman]

Intended as my first commit to the llvm-project.

Welcome! Thank you for working on this!

In D59859#1444176, @lebedev.ri wrote:

Please always upload all patches with full context (-U99999)
I'm not sure why we want this? What is wrong with simply applying clang-tidy twice?

It doubles the execution time of checking a large project (which may be unacceptably slow), and there's no guarantee that twice will be enough in the general case (one set of fixes may trigger a different check's diagnostics, which may get fixed and trigger another check's diagnostics, etc).

However, it seems like we can run into this sort of interaction in many different checks and in many different ways, and I am not certain that we should try to tackle the maintenance burden of dealing with those interactions ad hoc like this. That said, I'm not certain of a more principled solution either. Adding @alexfh to see if he has put any thought into this area and has ideas.

[MyDeveloperDay]

We normally add something to the documentation about the checker and/or the release notes to say what had changed

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

48

LLVM normally doesn't put braces on small lines

221

your don't really need the else here because you return above it

[alexfh]

In D59859#1444333, @aaron.ballman wrote:

In D59859#1444176, @lebedev.ri wrote:

I'm not sure why we want this? What is wrong with simply applying clang-tidy twice?

It doubles the execution time of checking a large project (which may be unacceptably slow), and there's no guarantee that twice will be enough in the general case (one set of fixes may trigger a different check's diagnostics, which may get fixed and trigger another check's diagnostics, etc).

Yeah, the situation when one automated fix generates another warning is not a nice user experience. It's not only about clang-tidy run time, it's also about annoying users and making their workflow less efficient. Thus it's better to generate clang-tidy-clean code in fixes, where possible and not particularly difficult to implement.

As for the readability-uppercase-literal-suffix check (, I wonder whether clang-format could do this? Non-whitespace changes were historically not desired in clang-format, but there are a number of features there that generate non-whitespace changes (e.g. comment formatting, string literal splitting, #include sorting, macro formatting). This one seems to be local and quite safe. Maybe propose this feature to clang-format?

If the clang-format feature is viable, clang-tidy can already apply clang-format formatting. If not, I'm also not opposed to the new option for readability-implicit-bool-conversion (but maybe it should be a global option like StrictMode)?

[alexfh]

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

48

Maybe use a conditional operator instead?

return Type->isUnsignedIntegerType() ? (UppercaseSuffix ? "0U" : "0u") : "0";

[alexfh]

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

270

Relying on enabled checks for this sort of a behavior is undesired. This sort of an implicit dependency will lead to problems, if, for example, a user wants to apply fixes check-by-check. A separate option (local for this check or better a global one - like StrictMode) would be better.

[pnappa]

Apologies for taking a while, I'm afraid I hadn't had a moment of spare time over the past few weeks.

I believe I've applied the requested changes, bar one that I'm unsure about:

@alexfh to be sure, would adding an Option to "readability-implicit-bool-conversion" called say, "EnforceUppercase", suffice? With that option enabled the synthesis of the explicit comparison would be uppercase.

I'll make the appropriate changes to documentation when I get everything finalised.

[lebedev.ri]

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

270

Looks like this wasn't resolved - it should still be a config option, not hardcoded.

[lebedev.ri]

This review seems to be stuck/dead, consider abandoning if no longer relevant.

clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.h

private:
   void handleCastToBool(const ImplicitCastExpr *CastExpression,
                         const Stmt *ParentStatement, ASTContext &Context);
   void handleCastFromBool(const ImplicitCastExpr *CastExpression,
                           const ImplicitCastExpr *FurtherImplicitCastExpression,
                           ASTContext &Context);
 
   const bool AllowIntegerConditions;
   const bool AllowPointerConditions;
+  const bool UseUppercaseLiteralSuffix;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_IMPLICIT_BOOL_CONVERSION_H

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

 }
 
 AST_MATCHER(Stmt, isNULLMacroExpansion) {
   return isNULLMacroExpansion(&Node, Finder->getASTContext());
 }
 
 StringRef getZeroLiteralToCompareWithForType(CastKind CastExprKind,
                                              QualType Type,
-                                             ASTContext &Context) {
+                                             ASTContext &Context,
+                                             bool UppercaseSuffix) {
   switch (CastExprKind) {
   case CK_IntegralToBoolean:
-    return Type->isUnsignedIntegerType() ? "0u" : "0";
+    return Type->isUnsignedIntegerType() ? (UppercaseSuffix ? "0U" : "0u") : "0";
 
   case CK_FloatingToBoolean:
-    return Context.hasSameType(Type, Context.FloatTy) ? "0.0f" : "0.0";
+    return Context.hasSameType(Type, Context.FloatTy) ? (UppercaseSuffix ? "0.0F" : "0.0f") : "0.0";
 
   case CK_PointerToBoolean:
   case CK_MemberPointerToBoolean: // Fall-through on purpose.
     return Context.getLangOpts().CPlusPlus11 ? "nullptr" : "0";
 
   default:
     llvm_unreachable("Unexpected cast kind");
   }
if (const auto *OperatorCall = dyn_cast<CXXOperatorCallExpr>(Statement)) {
     return areParensNeededForOverloadedOperator(OperatorCall->getOperator());
   }
 
   return isa<BinaryOperator>(Statement) || isa<UnaryOperator>(Statement);
 }
 
 void fixGenericExprCastToBool(DiagnosticBuilder &Diag,
                               const ImplicitCastExpr *Cast, const Stmt *Parent,
-                              ASTContext &Context) {
+                              ASTContext &Context, bool UppercaseSuffix) {
   // In case of expressions like (! integer), we should remove the redundant not
   // operator and use inverted comparison (integer == 0).
   bool InvertComparison =
       Parent != nullptr && isUnaryLogicalNotOperator(Parent);
   if (InvertComparison) {
     SourceLocation ParentStartLoc = Parent->getBeginLoc();
     SourceLocation ParentEndLoc =
         cast<UnaryOperator>(Parent)->getSubExpr()->getBeginLoc();
void fixGenericExprCastToBool(DiagnosticBuilder &Diag,
 
   if (InvertComparison) {
     EndLocInsertion += " == ";
   } else {
     EndLocInsertion += " != ";
   }
 
   EndLocInsertion += getZeroLiteralToCompareWithForType(
-      Cast->getCastKind(), SubExpr->getType(), Context);
+      Cast->getCastKind(), SubExpr->getType(), Context, UppercaseSuffix);
 
   if (NeedOuterParens) {
     EndLocInsertion += ")";
   }
 
   SourceLocation EndLoc = Lexer::getLocForEndOfToken(
       Cast->getEndLoc(), 0, Context.getSourceManager(), Context.getLangOpts());
   Diag << FixItHint::CreateInsertion(EndLoc, EndLocInsertion);
SourceLocation EndLoc = Lexer::getLocForEndOfToken(
         Cast->getEndLoc(), 0, Context.getSourceManager(),
         Context.getLangOpts());
 
     Diag << FixItHint::CreateInsertion(EndLoc, ")");
   }
 }
 
 StringRef getEquivalentForBoolLiteral(const CXXBoolLiteralExpr *BoolLiteral,
-                                      QualType DestType, ASTContext &Context) {
+                                      QualType DestType, ASTContext &Context,
+                                      bool UppercaseSuffix) {
   // Prior to C++11, false literal could be implicitly converted to pointer.
   if (!Context.getLangOpts().CPlusPlus11 &&
       (DestType->isPointerType() || DestType->isMemberPointerType()) &&
       BoolLiteral->getValue() == false) {
     return "0";
   }
 
   if (DestType->isFloatingType()) {
     if (Context.hasSameType(DestType, Context.FloatTy)) {
+      if (UppercaseSuffix)
+        return BoolLiteral->getValue() ? "1.0F" : "0.0F";
       return BoolLiteral->getValue() ? "1.0f" : "0.0f";
     }
     return BoolLiteral->getValue() ? "1.0" : "0.0";
   }
 
   if (DestType->isUnsignedIntegerType()) {
+    if (UppercaseSuffix)
+      return BoolLiteral->getValue() ? "1U" : "0U";
     return BoolLiteral->getValue() ? "1u" : "0u";
   }
   return BoolLiteral->getValue() ? "1" : "0";
 }
 
 bool isCastAllowedInCondition(const ImplicitCastExpr *Cast,
                               ASTContext &Context) {
   std::queue<const Stmt *> Q;
   Q.push(Cast);

 }
 
 } // anonymous namespace
 
 ImplicitBoolConversionCheck::ImplicitBoolConversionCheck(
     StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       AllowIntegerConditions(Options.get("AllowIntegerConditions", false)),
-      AllowPointerConditions(Options.get("AllowPointerConditions", false)) {}
+      AllowPointerConditions(Options.get("AllowPointerConditions", false)),
+      UseUppercaseLiteralSuffix(Context->isCheckEnabled("readability-uppercase-literal-suffix")) {}
 
 void ImplicitBoolConversionCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "AllowIntegerConditions", AllowIntegerConditions);
   Options.store(Opts, "AllowPointerConditions", AllowPointerConditions);
 }
 
 void ImplicitBoolConversionCheck::registerMatchers(MatchFinder *Finder) {
   // This check doesn't make much sense if we run it on language without
   // built-in bool support.
   if (!getLangOpts().Bool) {
     return;
   }
 
   auto exceptionCases =
       expr(anyOf(allOf(isMacroExpansion(), unless(isNULLMacroExpansion())),
                  has(ignoringImplicit(memberExpr(hasDeclaration(fieldDecl(hasBitWidth(1)))))),
                  hasParent(explicitCastExpr())));
   auto implicitCastFromBool = implicitCastExpr(
       anyOf(hasCastKind(CK_IntegralCast), hasCastKind(CK_IntegralToFloating),
void ImplicitBoolConversionCheck::handleCastToBool(const ImplicitCastExpr *Cast,
   auto Diag = diag(Cast->getBeginLoc(), "implicit conversion %0 -> bool")
               << Cast->getSubExpr()->getType();
 
   StringRef EquivalentLiteral =
       getEquivalentBoolLiteralForExpr(Cast->getSubExpr(), Context);
   if (!EquivalentLiteral.empty()) {
     Diag << tooling::fixit::createReplacement(*Cast, EquivalentLiteral);
   } else {
-    fixGenericExprCastToBool(Diag, Cast, Parent, Context);
+    fixGenericExprCastToBool(Diag, Cast, Parent, Context, UseUppercaseLiteralSuffix);
   }
 }
 
 void ImplicitBoolConversionCheck::handleCastFromBool(
     const ImplicitCastExpr *Cast, const ImplicitCastExpr *NextImplicitCast,
     ASTContext &Context) {
   QualType DestType =
       NextImplicitCast ? NextImplicitCast->getType() : Cast->getType();
   auto Diag = diag(Cast->getBeginLoc(), "implicit conversion bool -> %0")
               << DestType;
 
   if (const auto *BoolLiteral =
           dyn_cast<CXXBoolLiteralExpr>(Cast->getSubExpr())) {
     Diag << tooling::fixit::createReplacement(
-        *Cast, getEquivalentForBoolLiteral(BoolLiteral, DestType, Context));
+        *Cast, getEquivalentForBoolLiteral(BoolLiteral, DestType, Context,
+                                           UseUppercaseLiteralSuffix));
   } else {
     fixGenericExprCastFromBool(Diag, Cast, Context, DestType.getAsString());
   }
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

clang-tools-extra/test/clang-tidy/readability-implicit-bool-conversion.cpp

void implicitConversionFromBoolInSingleBoolExpressions(bool b1, bool b2) {
   // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: implicit conversion bool -> 'float'
   // CHECK-FIXES: float floating = static_cast<float>(boolean) / 0.3f;
 
   char character = boolean;
   // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: implicit conversion bool -> 'char'
   // CHECK-FIXES: char character = static_cast<char>(boolean);
 }
 
-void implicitConversionFromBoollInComplexBoolExpressions() {
+void implicitConversionFromBoolInComplexBoolExpressions() {
   bool boolean = true;
   bool anotherBoolean = false;
 
   int integer = boolean && anotherBoolean;
   // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: implicit conversion bool -> 'int'
   // CHECK-FIXES: int integer = static_cast<int>(boolean && anotherBoolean);
 
   unsigned long unsignedLong = (! boolean) + 4ul;

clang-tools-extra/test/clang-tidy/readability-uppercase-literal-suffix-implicit-bool-conversion.cpp

+// RUN: %check_clang_tidy %s readability-uppercase-literal-suffix,readability-implicit-bool-conversion %t
+
+template<typename T>
+void functionTaking(T);
+
+
+////////// Implicit conversion to bool with enforced uppercase suffix.
+
+void implicitConversionToBoolSimpleCases() {
+  unsigned int unsignedInteger = 10u;
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: integer literal has suffix 'u', which is not uppercase
+  // CHECK-MESSAGES-NEXT: unsigned int unsignedInteger = 10u;
+  // CHECK-MESSAGES-NEXT: ^ ~
+  // CHECK-MESSAGES-NEXT: {{^ *}}U{{$}}
+  // CHECK-FIXES: unsigned int unsignedInteger = 10U;
+
+  functionTaking<bool>(unsignedInteger);
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'unsigned int' -> bool [readability-implicit-bool-conversion]
+  // CHECK-FIXES: functionTaking<bool>(unsignedInteger != 0U);
+
+  unsigned long unsignedLong = 10ul;
+  // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: integer literal has suffix 'ul', which is not uppercase [readability-uppercase-literal-suffix]
+  // CHECK-MESSAGES-NEXT: unsigned long unsignedLong = 10ul;
+  // CHECK-MESSAGES-NEXT: ^ ~~
+  // CHECK-MESSAGES-NEXT: {{^ *}}UL{{$}}
+  // CHECK-FIXES: unsigned long unsignedLong = 10UL;
+
+  functionTaking<bool>(unsignedLong);
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'unsigned long' -> bool [readability-implicit-bool-conversion]
+  // CHECK-FIXES: functionTaking<bool>(unsignedLong != 0U);
+
+  float floating = 10.0f;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: floating point literal has suffix 'f', which is not uppercase [readability-uppercase-literal-suffix]
+  // CHECK-MESSAGES-NEXT: float floating = 10.0f;
+  // CHECK-MESSAGES-NEXT: ^   ~
+  // CHECK-MESSAGES-NEXT: {{^ *}}F{{$}}
+  // CHECK-FIXES: float floating = 10.0F;
+
+  functionTaking<bool>(floating);
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'float' -> bool [readability-implicit-bool-conversion]
+  // CHECK-FIXES: functionTaking<bool>(floating != 0.0F);
+
+  double doubleFloating = 10.0;
+  functionTaking<bool>(doubleFloating);
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'double' -> bool [readability-implicit-bool-conversion]
+  // CHECK-FIXES: functionTaking<bool>(doubleFloating != 0.0);
+}
+
+void implicitConversionToBoolInComplexExpressions() {
+  bool boolean = true;
+
+  unsigned int integer = 10U;
+  unsigned int anotherInteger = 20U;
+  bool boolComingFromUnsignedInteger = integer + anotherInteger;
+  // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: implicit conversion 'unsigned int' -> bool
+  // CHECK-FIXES: bool boolComingFromUnsignedInteger = (integer + anotherInteger) != 0U;
+
+  float floating = 0.2F;
+  // combination of two errors on the same line
+  bool boolComingFromFloating = floating - 0.3f || boolean;
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: implicit conversion 'float' -> bool
+  // CHECK-MESSAGES: :[[@LINE-2]]:44: warning: floating point literal has suffix 'f', which is not uppercase [readability-uppercase-literal-suffix]
+  // CHECK-FIXES: bool boolComingFromFloating = ((floating - 0.3F) != 0.0F) || boolean;
+}
+
+void implicitConversionInNegationExpressions() {
+  // ensure that in negation the replaced suffix is also capitalized
+  float floating = 10.0F;
+  bool boolComingFromNegatedFloat = ! floating;
+  // CHECK-MESSAGES: :[[@LINE-1]]:39: warning: implicit conversion 'float' -> bool
+  // CHECK-FIXES: bool boolComingFromNegatedFloat = floating == 0.0F;
+}
+
+void implicitConversionToBoolInControlStatements() {
+  unsigned long longInteger = 2U;
+  for (;longInteger;) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: implicit conversion 'unsigned long' -> bool
+  // CHECK-FIXES: for (;longInteger != 0U;) {}
+
+  float floating = 0.3F;
+  while (floating) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: implicit conversion 'float' -> bool
+  // CHECK-FIXES: while (floating != 0.0F) {}
+}
+
+void implicitConversionToBoolFromUnaryOperatorLiterals() {
+  functionTaking<bool>(-0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'int' -> bool
+  // CHECK-FIXES: functionTaking<bool>((-0) != 0);
+
+  functionTaking<bool>(-0.0F);
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'float' -> bool
+  // CHECK-FIXES: functionTaking<bool>((-0.0F) != 0.0F);
+
+  functionTaking<bool>(-0.0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'double' -> bool
+  // CHECK-FIXES: functionTaking<bool>((-0.0) != 0.0);
+}
+
+bool implicitConversionToBoolFromUnsignedInReturnValue() {
+  unsigned integer = 1u;
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: integer literal has suffix 'u', which is not uppercase
+  // CHECK-MESSAGES-NEXT: unsigned integer = 1u;
+  // CHECK-MESSAGES-NEXT: ^~
+  // CHECK-MESSAGES-NEXT: {{^ *}}U{{$}}
+  // CHECK-FIXES: unsigned integer = 1U;
+
+  return integer;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: implicit conversion 'unsigned int' -> bool [readability-implicit-bool-conversion]
+  // CHECK-FIXES: return integer != 0U;
+}
+
+bool implicitConversionToBoolFromFloatInReturnValue() {
+  float floating = 1.0f;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: floating point literal has suffix 'f', which is not uppercase [readability-uppercase-literal-suffix]
+  // CHECK-MESSAGES-NEXT: float floating = 1.0f;
+  // CHECK-MESSAGES-NEXT: ^  ~
+  // CHECK-MESSAGES-NEXT: {{^ *}}F{{$}}
+  // CHECK-FIXES: float floating = 1.0F;
+
+  return floating;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: implicit conversion 'float' -> bool [readability-implicit-bool-conversion]
+  // CHECK-FIXES: return floating != 0.0F;
+}