From d8e78022c63b9fc9af6260eef667231c929e9cee Mon Sep 17 00:00:00 2001 From: Clement Courbet Date: Mon, 25 Mar 2019 08:18:00 +0000 Subject: [PATCH] [clang-tidy] Fix more false positives for bugprone-string-integer-assignment Summary: And add various tests gleaned for our codebase. See PR27723. Reviewers: JonasToth, alexfh, xazax.hun Subscribers: rnkovacs, jdoerfert, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D59360 llvm-svn: 356871 --- .../bugprone/StringIntegerAssignmentCheck.cpp | 107 ++++++++++++++++----- .../bugprone-string-integer-assignment.cpp | 42 +++++++- 2 files changed, 125 insertions(+), 24 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp index 389ddc1..c64b5f3 100644 --- a/clang-tools-extra/clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp @@ -45,38 +45,100 @@ void StringIntegerAssignmentCheck::registerMatchers(MatchFinder *Finder) { this); } -static bool isLikelyCharExpression(const Expr *Argument, - const ASTContext &Ctx) { - const auto *BinOp = dyn_cast(Argument); - if (!BinOp) +class CharExpressionDetector { +public: + CharExpressionDetector(QualType CharType, const ASTContext &Ctx) + : CharType(CharType), Ctx(Ctx) {} + + bool isLikelyCharExpression(const Expr *E) const { + if (isCharTyped(E)) + return true; + + if (const auto *BinOp = dyn_cast(E)) { + const auto *LHS = BinOp->getLHS()->IgnoreParenImpCasts(); + const auto *RHS = BinOp->getRHS()->IgnoreParenImpCasts(); + // Handle both directions, e.g. `'a' + (i % 26)` and `(i % 26) + 'a'`. + if (BinOp->isAdditiveOp() || BinOp->isBitwiseOp()) + return handleBinaryOp(BinOp->getOpcode(), LHS, RHS) || + handleBinaryOp(BinOp->getOpcode(), RHS, LHS); + // Except in the case of '%'. + if (BinOp->getOpcode() == BO_Rem) + return handleBinaryOp(BinOp->getOpcode(), LHS, RHS); + return false; + } + + // Ternary where at least one branch is a likely char expression, e.g. + // i < 265 ? i : ' ' + if (const auto *CondOp = dyn_cast(E)) + return isLikelyCharExpression( + CondOp->getFalseExpr()->IgnoreParenImpCasts()) || + isLikelyCharExpression( + CondOp->getTrueExpr()->IgnoreParenImpCasts()); return false; - const auto *LHS = BinOp->getLHS()->IgnoreParenImpCasts(); - const auto *RHS = BinOp->getRHS()->IgnoreParenImpCasts(); - // & , mask is a compile time constant. - Expr::EvalResult RHSVal; - if (BinOp->getOpcode() == BO_And && - (RHS->EvaluateAsInt(RHSVal, Ctx, Expr::SE_AllowSideEffects) || - LHS->EvaluateAsInt(RHSVal, Ctx, Expr::SE_AllowSideEffects))) - return true; - // + ( % ), where is a char literal. - const auto IsCharPlusModExpr = [](const Expr *L, const Expr *R) { - const auto *ROp = dyn_cast(R); - return ROp && ROp->getOpcode() == BO_Rem && isa(L); - }; - if (BinOp->getOpcode() == BO_Add) { - if (IsCharPlusModExpr(LHS, RHS) || IsCharPlusModExpr(RHS, LHS)) + } + +private: + bool handleBinaryOp(clang::BinaryOperatorKind Opcode, const Expr *const LHS, + const Expr *const RHS) const { + // (c++ integer promotion rules make this an + // int), e.g. + // 'a' + c + if (isCharTyped(LHS) && isCharTyped(RHS)) + return true; + + // & or % , e.g. + // i & 0xff + if ((Opcode == BO_And || Opcode == BO_Rem) && isCharValuedConstant(RHS)) + return true; + + // | , e.g. + // c | 0x80 + if (Opcode == BO_Or && isCharTyped(LHS) && isCharValuedConstant(RHS)) return true; + + // + , e.g. + // 'a' + (i % 26) + if (Opcode == BO_Add) + return isCharConstant(LHS) && isLikelyCharExpression(RHS); + + return false; } - return false; -} + + // Returns true if `E` is an character constant. + bool isCharConstant(const Expr *E) const { + return isCharTyped(E) && isCharValuedConstant(E); + }; + + // Returns true if `E` is an integer constant which fits in `CharType`. + bool isCharValuedConstant(const Expr *E) const { + if (E->isInstantiationDependent()) + return false; + Expr::EvalResult EvalResult; + if (!E->EvaluateAsInt(EvalResult, Ctx, Expr::SE_AllowSideEffects)) + return false; + return EvalResult.Val.getInt().getActiveBits() <= Ctx.getTypeSize(CharType); + }; + + // Returns true if `E` has the right character type. + bool isCharTyped(const Expr *E) const { + return E->getType().getCanonicalType().getTypePtr() == + CharType.getTypePtr(); + }; + + const QualType CharType; + const ASTContext &Ctx; +}; void StringIntegerAssignmentCheck::check( const MatchFinder::MatchResult &Result) { const auto *Argument = Result.Nodes.getNodeAs("expr"); + const auto CharType = + Result.Nodes.getNodeAs("type")->getCanonicalType(); SourceLocation Loc = Argument->getBeginLoc(); // Try to detect a few common expressions to reduce false positives. - if (isLikelyCharExpression(Argument, *Result.Context)) + if (CharExpressionDetector(CharType, *Result.Context) + .isLikelyCharExpression(Argument)) return; auto Diag = @@ -88,7 +150,6 @@ void StringIntegerAssignmentCheck::check( if (Loc.isMacroID()) return; - auto CharType = *Result.Nodes.getNodeAs("type"); bool IsWideCharType = CharType->isWideCharType(); if (!CharType->isCharType() && !IsWideCharType) return; diff --git a/clang-tools-extra/test/clang-tidy/bugprone-string-integer-assignment.cpp b/clang-tools-extra/test/clang-tidy/bugprone-string-integer-assignment.cpp index dbf3a5c..18fe5ef 100644 --- a/clang-tools-extra/test/clang-tidy/bugprone-string-integer-assignment.cpp +++ b/clang-tools-extra/test/clang-tidy/bugprone-string-integer-assignment.cpp @@ -7,6 +7,8 @@ struct basic_string { basic_string& operator=(basic_string); basic_string& operator+=(T); basic_string& operator+=(basic_string); + const T &operator[](int i) const; + T &operator[](int i); }; typedef basic_string string; @@ -21,10 +23,13 @@ int toupper(int i); typedef int MyArcaneChar; +constexpr char kCharConstant = 'a'; + int main() { std::string s; std::wstring ws; int x = 5; + const char c = 'c'; s = 6; // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: an integer is interpreted as a character code when assigning {{.*}} [bugprone-string-integer-assignment] @@ -58,12 +63,47 @@ int main() { s += toupper(x); s += tolower(x); - s += std::tolower(x); + s += (std::tolower(x)); + + s += c & s[1]; + s += c ^ s[1]; + s += c | s[1]; + + s[x] += 1; + s += s[x]; + as += as[x]; // Likely character expressions. s += x & 0xff; s += 0xff & x; + s += x % 26; + s += 26 % x; + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: an integer is interpreted as a chara + // CHECK-FIXES: {{^}} s += std::to_string(26 % x);{{$}} + s += c | 0x80; + s += c | 0x8000; + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: an integer is interpreted as a chara + // CHECK-FIXES: {{^}} s += std::to_string(c | 0x8000);{{$}} + as += c | 0x8000; s += 'a' + (x % 26); + s += kCharConstant + (x % 26); + s += 'a' + (s[x] & 0xf); s += (x % 10) + 'b'; + + s += x > 255 ? c : x; + s += x > 255 ? 12 : x; + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: an integer is interpreted as a chara + // CHECK-FIXES: {{^}} s += std::to_string(x > 255 ? 12 : x);{{$}} +} + +namespace instantiation_dependent_exprs { +template +struct S { + static constexpr T t = 0x8000; + std::string s; + void f(char c) { s += c | static_cast(t); } +}; + +template S; } -- 2.7.4