diff --git a/clang-tools-extra/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp --- a/clang-tools-extra/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp @@ -25,6 +25,19 @@ 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), @@ -50,8 +63,9 @@ /// 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. @@ -93,20 +107,27 @@ } /// 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, + const QualType &UpperBoundType) { // Ignore casting caused by constant values inside a binary operator. // We are interested in variable values' magnitude bits. if (const auto *BinOperator = dyn_cast(UpperBound)) { @@ -117,7 +138,7 @@ QualType LHSEType = LHSE->getType(); if (!RHSEType->isIntegerType() || !LHSEType->isIntegerType()) - return 0; + return {}; bool RHSEIsConstantValue = RHSEType->isEnumeralType() || RHSEType.isConstQualified() || @@ -128,17 +149,28 @@ // 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), - calcMagnitudeBits(Context, RHSEType)); + return std::max(calcMagnitudeBits(Context, LHSEType, LHSE), + calcMagnitudeBits(Context, RHSEType, RHSE)); } - return calcMagnitudeBits(Context, UpperBoundType); + return calcMagnitudeBits(Context, UpperBoundType, UpperBound); +} + +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) { @@ -152,25 +184,26 @@ 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 diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -178,6 +178,10 @@ ``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 + ` check. Basic support + for bit-field and integer members as a loop variable or upper limit were added. + Removed checks ^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/too-small-loop-variable.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/too-small-loop-variable.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/too-small-loop-variable.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/too-small-loop-variable.cpp @@ -46,6 +46,16 @@ } } +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) { @@ -253,3 +263,113 @@ 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] + } +} +