[clang-tidy] Improved too-small-loop-variable with bit-field support

[PiotrZSL]

Summary

Implemented support for bit-field members as a loop variable
or upper limit. Supporting also non bit-field integer members.

Fixes issues: https://github.com/llvm/llvm-project/issues/58614

[PiotrZSL]

Ready for review.

[carlosgalvezp]

Thanks for the fix! I have some suggestions for improved readability.

clang-tools-extra/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp

109–110

Having an std::pair is a bit hard to read, as it's not clear what each element of the pair represents. Could you replace it with something like this, for improved readability and maintainability? Then you can also skip the utility header.

struct MagnitudeBits
{
  unsigned Width;
  bool IsBitField;
};

204

This is a bit confusing, could you keep it with %0 and %1 (which clearly represent "loop variable" and "upper bound"), and simply create a std::string with the appropriate content? Something like:

std::string type = LoopVarType.getAsString();
if (LoopVarMagnitudeBits.second) {
  type += ":" + std::to_string(LoopVarMagntiudeBits.second);
}
Msg << type;

215–220

Please create a helper function to remove duplication.

[Eugene.Zelenko]

@carlosgalvezp: Sorry, there are too much Clang specifics in this patch, so I could not be reviewer.

[carlosgalvezp]

In D142587#4152943, @Eugene.Zelenko wrote:

@carlosgalvezp: Sorry, there are too much Clang specifics in this patch, so I could not be reviewer.

No prob, thanks for letting us know :)

[PiotrZSL]

Review comments fix

[carlosgalvezp]

LGTM, thanks!

[Henric]

Hi
With this patch, the following code gives me a warning that I don't think is intentional.

typedef struct S {
  int x:4;
} S;

void foo(S s) {
  for (int i=10; i > s.x; --i) ;
}

loop_var.c:6:22: warning: loop variable has narrower type 'int:4' than iteration's upper bound 'int' [bugprone-too-small-loop-variable]
for (int i=10; i > s.x; --i) ;
                   ^

Looks like the logic i reversed here, so the loop variable gets the bit field type.

[PiotrZSL]

Thank you for information, I will look into this....
Indeed test for this scenario is missing.

clang-tools-extra/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp

static constexpr llvm::StringLiteral LoopVarName =
     llvm::StringLiteral("loopVar");
 static constexpr llvm::StringLiteral LoopVarCastName =
     llvm::StringLiteral("loopVarCast");
 static constexpr llvm::StringLiteral LoopUpperBoundName =
     llvm::StringLiteral("loopUpperBound");
 static constexpr llvm::StringLiteral LoopIncrementName =
     llvm::StringLiteral("loopIncrement");
 
+namespace {
+
+struct MagnitudeBits {
+  unsigned WidthWithoutSignBit = 0U;
+  unsigned BitFieldWidth = 0U;
+
+  bool operator<(const MagnitudeBits &Other) const noexcept {
+    return WidthWithoutSignBit < Other.WidthWithoutSignBit;
+  }
+};
+
+} // namespace
+
 TooSmallLoopVariableCheck::TooSmallLoopVariableCheck(StringRef Name,
                                                      ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       MagnitudeBitsUpperLimit(Options.get("MagnitudeBitsUpperLimit", 16U)) {}
 
 void TooSmallLoopVariableCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "MagnitudeBitsUpperLimit", MagnitudeBitsUpperLimit);

 ///   LoopVarName: 'j' (as a VarDecl)
 ///   LoopVarCastName: 'j' (after implicit conversion)
 ///   LoopUpperBoundName: '3 + 2' (as an Expr)
 ///   LoopIncrementName: 'k' (as an Expr)
 ///   LoopName: The entire for loop (as a ForStmt)
 ///
 void TooSmallLoopVariableCheck::registerMatchers(MatchFinder *Finder) {
   StatementMatcher LoopVarMatcher =
-      expr(
-          ignoringParenImpCasts(declRefExpr(to(varDecl(hasType(isInteger()))))))
+      expr(ignoringParenImpCasts(
+               anyOf(declRefExpr(to(varDecl(hasType(isInteger())))),
+                     memberExpr(member(fieldDecl(hasType(isInteger())))))))
           .bind(LoopVarName);
 
   // We need to catch only those comparisons which contain any integer cast.
   StatementMatcher LoopVarConversionMatcher = traverse(
       TK_AsIs, implicitCastExpr(hasImplicitDestinationType(isInteger()),
                                 has(ignoringParenImpCasts(LoopVarMatcher)))
                    .bind(LoopVarCastName));
 
Finder->addMatcher(
                              hasRHS(LoopVarConversionMatcher)),
               binaryOperator(hasOperatorName(">="), hasLHS(LoopBoundMatcher),
                              hasRHS(LoopVarConversionMatcher)))),
           hasIncrement(IncrementMatcher))
           .bind(LoopName),
       this);
 }
 
 /// Returns the magnitude bits of an integer type.
-static unsigned calcMagnitudeBits(const ASTContext &Context,
-                                  const QualType &IntExprType) {
+static MagnitudeBits calcMagnitudeBits(const ASTContext &Context,
+                                       const QualType &IntExprType,
+                                       const Expr *IntExpr) {
   assert(IntExprType->isIntegerType());
 
-  return IntExprType->isUnsignedIntegerType()
-             ? Context.getIntWidth(IntExprType)
-             : Context.getIntWidth(IntExprType) - 1;
+  unsigned SignedBits = IntExprType->isUnsignedIntegerType() ? 0U : 1U;
+
+  if (const auto *BitField = IntExpr->getSourceBitField()) {
+    unsigned BitFieldWidth = BitField->getBitWidthValue(Context);
+    return {BitFieldWidth - SignedBits, BitFieldWidth};
+  }
+
+  unsigned IntWidth = Context.getIntWidth(IntExprType);
+  return {IntWidth - SignedBits, 0U};
 }
 
 /// Calculate the upper bound expression's magnitude bits, but ignore
 /// constant like values to reduce false positives.
-static unsigned calcUpperBoundMagnitudeBits(const ASTContext &Context,
-                                            const Expr *UpperBound,
-                                            const QualType &UpperBoundType) {
+static MagnitudeBits
+calcUpperBoundMagnitudeBits(const ASTContext &Context, const Expr *UpperBound,
   // Ignore casting caused by constant values inside a binary operator.
   // We are interested in variable values' magnitude bits.
   if (const auto *BinOperator = dyn_cast<BinaryOperator>(UpperBound)) {
     const Expr *RHSE = BinOperator->getRHS()->IgnoreParenImpCasts();
     const Expr *LHSE = BinOperator->getLHS()->IgnoreParenImpCasts();
 
     QualType RHSEType = RHSE->getType();
     QualType LHSEType = LHSE->getType();
 
     if (!RHSEType->isIntegerType() || !LHSEType->isIntegerType())
-      return 0;
+      return {};
 
     bool RHSEIsConstantValue = RHSEType->isEnumeralType() ||
                                RHSEType.isConstQualified() ||
                                isa<IntegerLiteral>(RHSE);
     bool LHSEIsConstantValue = LHSEType->isEnumeralType() ||
                                LHSEType.isConstQualified() ||
                                isa<IntegerLiteral>(LHSE);
 
     // Avoid false positives produced by two constant values.
     if (RHSEIsConstantValue && LHSEIsConstantValue)
-      return 0;
+      return {};
     if (RHSEIsConstantValue)
-      return calcMagnitudeBits(Context, LHSEType);
+      return calcMagnitudeBits(Context, LHSEType, LHSE);
     if (LHSEIsConstantValue)
-      return calcMagnitudeBits(Context, RHSEType);
+      return calcMagnitudeBits(Context, RHSEType, RHSE);
+
+    return std::max(calcMagnitudeBits(Context, LHSEType, LHSE),
+                    calcMagnitudeBits(Context, RHSEType, RHSE));
+  }
 
-    return std::max(calcMagnitudeBits(Context, LHSEType),
-                    calcMagnitudeBits(Context, RHSEType));
-  }
+  return calcMagnitudeBits(Context, UpperBoundType, UpperBound);
 
-  return calcMagnitudeBits(Context, UpperBoundType);
+static std::string formatIntegralType(const QualType &Type,
+                                      const MagnitudeBits &Info) {
+  std::string Name = Type.getAsString();
+  if (!Info.BitFieldWidth)
+    return Name;
+
+  Name += ':';
+  Name += std::to_string(Info.BitFieldWidth);
+  return Name;
 }
 
 void TooSmallLoopVariableCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *LoopVar = Result.Nodes.getNodeAs<Expr>(LoopVarName);
   const auto *UpperBound =
       Result.Nodes.getNodeAs<Expr>(LoopUpperBoundName)->IgnoreParenImpCasts();
   const auto *LoopIncrement =
       Result.Nodes.getNodeAs<Expr>(LoopIncrementName)->IgnoreParenImpCasts();
 
   // We matched the loop variable incorrectly.
   if (LoopVar->getType() != LoopIncrement->getType())
     return;
 
-  QualType LoopVarType = LoopVar->getType();
-  QualType UpperBoundType = UpperBound->getType();
+  const QualType LoopVarType = LoopVar->getType();
+  const QualType UpperBoundType = UpperBound->getType();
 
   ASTContext &Context = *Result.Context;
 
-  unsigned LoopVarMagnitudeBits = calcMagnitudeBits(Context, LoopVarType);
-  unsigned UpperBoundMagnitudeBits =
+  const MagnitudeBits LoopVarMagnitudeBits =
+      calcMagnitudeBits(Context, LoopVarType, LoopVar);
+  const MagnitudeBits UpperBoundMagnitudeBits =
       calcUpperBoundMagnitudeBits(Context, UpperBound, UpperBoundType);
 
-  if (UpperBoundMagnitudeBits == 0)
-    return;
-
-  if (LoopVarMagnitudeBits > MagnitudeBitsUpperLimit)
+  if ((0U == UpperBoundMagnitudeBits.WidthWithoutSignBit) ||
+      (LoopVarMagnitudeBits.WidthWithoutSignBit > MagnitudeBitsUpperLimit) ||
+      (LoopVarMagnitudeBits.WidthWithoutSignBit >=
+       UpperBoundMagnitudeBits.WidthWithoutSignBit))
     return;
 
-  if (LoopVarMagnitudeBits < UpperBoundMagnitudeBits)
-    diag(LoopVar->getBeginLoc(), "loop variable has narrower type %0 than "
-                                 "iteration's upper bound %1")
-        << LoopVarType << UpperBoundType;
+  diag(LoopVar->getBeginLoc(),
+       "loop variable has narrower type '%0' than iteration's upper bound '%1'")
+      << formatIntegralType(LoopVarType, LoopVarMagnitudeBits)
+      << formatIntegralType(UpperBoundType, UpperBoundMagnitudeBits);
 }
 
 } // namespace clang::tidy::bugprone

clang-tools-extra/docs/ReleaseNotes.rst

- Renamed `HungarianNotation.CString` options `CharPrinter` and
   :doc:`readability-identifier-naming
   <clang-tidy/checks/readability/identifier-naming>` check.
 
 - Fixed a false positive in :doc:`readability-container-size-empty
   <clang-tidy/checks/readability/container-size-empty>` check when comparing
   ``std::array`` objects to default constructed ones. The behavior for this and
   other relevant classes can now be configured with a new option.
 
+- Improved :doc:`bugprone-too-small-loop-variable
+  <clang-tidy/checks/bugprone/too-small-loop-variable>` check. Basic support
+  for bit-field and integer members as a loop variable or upper limit were added.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

clang-tools-extra/test/clang-tidy/checkers/bugprone/too-small-loop-variable.cpp

 
 void voidBadForLoop6() {
   int i = 0;
   for (; i < size(); ++i) {
     // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
   }
 }
 
+void voidBadForLoop7() {
+    struct Int  {
+        int value;
+    } i;
+
+  for (i.value = 0; i.value < size(); ++i.value) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: loop variable has narrower type 'int' than iteration's upper bound 'long' [bugprone-too-small-loop-variable]
+  }
+}
+
 void voidForLoopUnsignedBound() {
   unsigned size = 3147483647;
   for (int i = 0; i < size; ++i) {
     // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: loop variable has narrower type 'int' than iteration's upper bound 'unsigned int' [bugprone-too-small-loop-variable]
   }
 }
 
 // The iteration's upper bound has a template dependent value.

 
 // The used constant variable leads to an infinite loop.
 // This is caught by -Wtautological-constant-out-of-range-compare.
 void voidForLoopWithBigConstBound() {
   const long size = 294967296l;
   for (short i = 0; i < size; ++i) { // no warning
   }
 }
+
+// Should detect proper size of upper bound bitfield
+void voidForLoopWithBitfieldOnUpperBound() {
+  struct StructWithBitField {
+      unsigned bitfield : 5;
+  } value = {};
+
+  for(unsigned char i = 0U; i < value.bitfield; ++i) { // no warning
+  }
+}
+
+// Should detect proper size of loop variable bitfield
+void voidForLoopWithBitfieldOnLoopVar() {
+  struct StructWithBitField {
+      unsigned bitfield : 9;
+  } value = {};
+
+  unsigned char upperLimit = 100U;
+
+  for(value.bitfield = 0U; value.bitfield < upperLimit; ++value.bitfield) {
+  }
+}
+
+// Should detect proper size of loop variable and upper bound
+void voidForLoopWithBitfieldOnLoopVarAndUpperBound() {
+  struct StructWithBitField {
+      unsigned var : 5, limit : 4;
+  } value = {};
+
+  for(value.var = 0U; value.var < value.limit; ++value.var) {
+  }
+}
+
+// Should detect proper size of loop variable and upper bound on integers
+void voidForLoopWithBitfieldOnLoopVarAndUpperBoundOnInt() {
+  struct StructWithBitField {
+      unsigned var : 5;
+      int limit : 6;
+  } value = {};
+
+  for(value.var = 0U; value.var < value.limit; ++value.var) {
+  }
+}
+
+void badForLoopWithBitfieldOnUpperBound() {
+  struct StructWithBitField {
+      unsigned bitfield : 9;
+  } value = {};
+
+  for(unsigned char i = 0U; i < value.bitfield; ++i) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: loop variable has narrower type 'unsigned char' than iteration's upper bound 'unsigned int:9' [bugprone-too-small-loop-variable]
+  }
+}
+
+void badForLoopWithBitfieldOnLoopVar() {
+  struct StructWithBitField {
+      unsigned bitfield : 7;
+  } value = {};
+
+  unsigned char upperLimit = 100U;
+
+  for(value.bitfield = 0U; value.bitfield < upperLimit; ++value.bitfield) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: loop variable has narrower type 'unsigned int:7' than iteration's upper bound 'unsigned char' [bugprone-too-small-loop-variable]
+  }
+}
+
+void badForLoopWithBitfieldOnLoopVarAndUpperBound() {
+  struct StructWithBitField {
+      unsigned var : 5, limit : 6;
+  } value = {};
+
+  for(value.var = 0U; value.var < value.limit; ++value.var) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: loop variable has narrower type 'unsigned int:5' than iteration's upper bound 'unsigned int:6' [bugprone-too-small-loop-variable]
+  }
+}
+
+void badForLoopWithBitfieldOnLoopVarOnIntAndUpperBound() {
+  struct StructWithBitField {
+      int var : 5;
+      unsigned limit : 5;
+  } value = {};
+
+  for(value.var = 0U; value.var < value.limit; ++value.var) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: loop variable has narrower type 'int:5' than iteration's upper bound 'unsigned int:5' [bugprone-too-small-loop-variable]
+  }
+}
+
+void badForLoopWithBitfieldOnLoopVarAndUpperBoundOnInt() {
+  struct StructWithBitField {
+      unsigned var : 5;
+      int limit : 7;
+  } value = {};
+
+  for(value.var = 0U; value.var < value.limit; ++value.var) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: loop variable has narrower type 'unsigned int:5' than iteration's upper bound 'int:7' [bugprone-too-small-loop-variable]
+  }
+}
+
+void badForLoopWithBitfieldOnLoopVarAndUpperBoundOnPtr() {
+  struct StructWithBitField {
+      unsigned var : 5, limit : 6;
+  } value = {};
+
+  StructWithBitField* ptr = &value;
+
+  for(ptr->var = 0U; ptr->var < ptr->limit; ++ptr->var) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: loop variable has narrower type 'unsigned int:5' than iteration's upper bound 'unsigned int:6' [bugprone-too-small-loop-variable]
+  }
+}
+