[clang-tidy] Ignore bool -> single bit bitfield conversion in readability-implicit-bool-conversion

[malcolm.parsons]

Summary

There is no ambiguity / information loss in this conversion

[aaron.ballman]

docs/clang-tidy/checks/readability-implicit-bool-conversion.rst

77–78 ↗

(On Diff #175438)

I think it should only be allowed if the bit-field type is unsigned; signed bit-fields with a single bit are inherently not portable because you don't know if that bit represents a sign bit or a value bit (imagine a sign and magnitude integer representation).

C++20 is changing this by standardizing on two's complement, but earlier versions of C++ (and currently, all versions of C) are still impacted, so another approach is to gate this on the language standard mode that's in effect.

[malcolm.parsons]

docs/clang-tidy/checks/readability-implicit-bool-conversion.rst

77–78 ↗

(On Diff #175438)

I think it's the responsibility of a compiler using sign and magnitude representation to warn about signed single bit bitfields.

[alexfh]

LG

docs/clang-tidy/checks/readability-implicit-bool-conversion.rst

77–78 ↗

(On Diff #175438)

I agree with Malcolm's argument. But if the concern is practical (i.e. if there's a user of this check, who's working with such compiler), we can add an option to enable the warning in this case. Any objections?

[aaron.ballman]

docs/clang-tidy/checks/readability-implicit-bool-conversion.rst

77–78 ↗

(On Diff #175438)

I think LLVM only supports two's complement backends currently anyway, so this is probably fine as-is. It was more a reaction to "there is no information loss" because there is information loss with the integer value, even in two's complement. It seems like clang is missing a warning that could handle this, however. https://godbolt.org/z/34xXIQ

Boolean assignments are a bit of a different beast in that the bool is converted to integer as either 0 or 1, explicitly. It's a bit strange that you put in true (which would be converted to 1) and get back out -1 as the integer value, but it's not awful because of the usual "0 is false, nonzero is true" conversion behavior back to bool.

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

Finder->addMatcher(
       this);
 
   auto boolComparison = binaryOperator(
       anyOf(hasOperatorName("=="), hasOperatorName("!=")),
       hasLHS(implicitCastFromBool), hasRHS(implicitCastFromBool));
   auto boolOpAssignment =
       binaryOperator(anyOf(hasOperatorName("|="), hasOperatorName("&=")),
                      hasLHS(expr(hasType(booleanType()))));
+  auto bitfieldAssignment = binaryOperator(
+      hasLHS(memberExpr(hasDeclaration(fieldDecl(hasBitWidth(1))))));
+  auto bitfieldConstruct = cxxConstructorDecl(hasDescendant(cxxCtorInitializer(
+      withInitializer(equalsBoundNode("implicitCastFromBool")),
+      forField(hasBitWidth(1)))));
   Finder->addMatcher(
       implicitCastExpr(
           implicitCastFromBool,
           // Exclude comparisons of bools, as they are always cast to integers
           // in such context:
           //   bool_expr_a == bool_expr_b
           //   bool_expr_a != bool_expr_b
-          unless(hasParent(binaryOperator(
-              anyOf(boolComparison, boolXor, boolOpAssignment)))),
+          unless(hasParent(binaryOperator(anyOf(
+              boolComparison, boolXor, boolOpAssignment, bitfieldAssignment)))),
+          implicitCastExpr().bind("implicitCastFromBool"),
+          unless(hasParent(bitfieldConstruct)),
           // Check also for nested casts, for example: bool -> int -> float.
           anyOf(hasParent(implicitCastExpr().bind("furtherImplicitCast")),
                 anything()),
           unless(isInTemplateInstantiation()),
-          unless(hasAncestor(functionTemplateDecl())))
-          .bind("implicitCastFromBool"),
+          unless(hasAncestor(functionTemplateDecl()))),
       this);
 }
 
 void ImplicitBoolConversionCheck::check(
     const MatchFinder::MatchResult &Result) {
   if (const auto *CastToBool =
           Result.Nodes.getNodeAs<ImplicitCastExpr>("implicitCastToBool")) {
     const auto *Parent = Result.Nodes.getNodeAs<Stmt>("parentStmt");

clang-tools-extra/trunk/docs/clang-tidy/checks/readability-implicit-bool-conversion.rst

 - integer expression/literal to boolean (conversion from a single bit bitfield
   to boolean is explicitly allowed, since there's no ambiguity / information
   loss in this case),
 
 - floating expression/literal to boolean,
 
 - pointer/pointer to member/``nullptr``/``NULL`` to boolean,
 
-- boolean expression/literal to integer,
+- boolean expression/literal to integer (conversion from boolean to a single
+  bit bitfield is explicitly allowed),
 
 - boolean expression/literal to floating.
 
 The rules for generating fix-it hints are:
 
 - in case of conversions from other built-in type to bool, an explicit
   comparison is proposed to make it clear what exaclty is being compared:
 

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

 } // namespace ignoreUserDefinedConversionOperator
 
 namespace ignore_1bit_bitfields {
 
 struct S {
   int a;
   int b : 1;
   int c : 2;
+
+  S(bool a, bool b, bool c) : a(a), b(b), c(c) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: implicit conversion bool -> 'int'
+  // CHECK-MESSAGES: :[[@LINE-2]]:45: warning: implicit conversion bool -> 'int'
+  // CHECK-FIXES: S(bool a, bool b, bool c) : a(static_cast<int>(a)), b(b), c(static_cast<int>(c)) {}
 };
 
-bool f(const S& s) {
+bool f(S& s) {
   functionTaking<bool>(s.a);
   // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'int' -> bool
   // CHECK-FIXES: functionTaking<bool>(s.a != 0);
   functionTaking<bool>(s.b);
   // CHECK-FIXES: functionTaking<bool>(s.b);
+  s.a = true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: implicit conversion bool -> 'int'
+  // CHECK-FIXES: s.a = 1;
+  s.b = true;
+  // CHECK-FIXES: s.b = true;
+  s.c = true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: implicit conversion bool -> 'int'
+  // CHECK-FIXES: s.c = 1;
   functionTaking<bool>(s.c);
   // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: implicit conversion 'int' -> bool
   // CHECK-FIXES: functionTaking<bool>(s.c != 0);
 }
 
 } // namespace ignore_1bit_bitfields