diff options
6 files changed, 233 insertions, 43 deletions
diff --git a/clang-tools-extra/clang-tidy/misc/BoolPointerImplicitConversionCheck.cpp b/clang-tools-extra/clang-tidy/misc/BoolPointerImplicitConversionCheck.cpp index ec13e63b918..7fd1ca0feb5 100644 --- a/clang-tools-extra/clang-tidy/misc/BoolPointerImplicitConversionCheck.cpp +++ b/clang-tools-extra/clang-tidy/misc/BoolPointerImplicitConversionCheck.cpp @@ -61,7 +61,8 @@ void BoolPointerImplicitConversionCheck::check( *Result.Context).empty() || // FIXME: We should still warn if the paremater is implicitly converted to // bool. - !match(findAll(callExpr(hasAnyArgument(DeclRef))), *If, *Result.Context) + !match(findAll(callExpr(hasAnyArgument(ignoringParenImpCasts(DeclRef)))), + *If, *Result.Context) .empty() || !match(findAll(cxxDeleteExpr(has(expr(DeclRef)))), *If, *Result.Context) .empty()) diff --git a/clang-tools-extra/clang-tidy/misc/MisplacedWideningCastCheck.cpp b/clang-tools-extra/clang-tidy/misc/MisplacedWideningCastCheck.cpp index a1666c5686f..c870da1ab04 100644 --- a/clang-tools-extra/clang-tidy/misc/MisplacedWideningCastCheck.cpp +++ b/clang-tools-extra/clang-tidy/misc/MisplacedWideningCastCheck.cpp @@ -10,6 +10,7 @@ #include "MisplacedWideningCastCheck.h" #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "llvm/ADT/DenseMap.h" using namespace clang::ast_matchers; @@ -17,6 +18,16 @@ namespace clang { namespace tidy { namespace misc { +MisplacedWideningCastCheck::MisplacedWideningCastCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + CheckImplicitCasts(Options.get("CheckImplicitCasts", true)) {} + +void MisplacedWideningCastCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "CheckImplicitCasts", CheckImplicitCasts); +} + void MisplacedWideningCastCheck::registerMatchers(MatchFinder *Finder) { auto Calc = expr(anyOf(binaryOperator(anyOf( hasOperatorName("+"), hasOperatorName("-"), @@ -25,14 +36,22 @@ void MisplacedWideningCastCheck::registerMatchers(MatchFinder *Finder) { hasType(isInteger())) .bind("Calc"); - auto Cast = explicitCastExpr(anyOf(cStyleCastExpr(), cxxStaticCastExpr(), - cxxReinterpretCastExpr()), - hasDestinationType(isInteger()), has(Calc)) - .bind("Cast"); + auto ExplicitCast = + explicitCastExpr(hasDestinationType(isInteger()), has(Calc)); + auto ImplicitCast = + implicitCastExpr(hasImplicitDestinationType(isInteger()), has(Calc)); + auto Cast = expr(anyOf(ExplicitCast, ImplicitCast)).bind("Cast"); - Finder->addMatcher(varDecl(has(Cast)), this); - Finder->addMatcher(returnStmt(has(Cast)), this); + Finder->addMatcher(varDecl(hasInitializer(Cast)), this); + Finder->addMatcher(returnStmt(hasReturnValue(Cast)), this); + Finder->addMatcher(callExpr(hasAnyArgument(Cast)), this); Finder->addMatcher(binaryOperator(hasOperatorName("="), hasRHS(Cast)), this); + Finder->addMatcher( + binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!="), + hasOperatorName("<"), hasOperatorName("<="), + hasOperatorName(">"), hasOperatorName(">=")), + anyOf(hasLHS(Cast), hasRHS(Cast))), + this); } static unsigned getMaxCalculationWidth(ASTContext &Context, const Expr *E) { @@ -75,8 +94,72 @@ static unsigned getMaxCalculationWidth(ASTContext &Context, const Expr *E) { return Context.getIntWidth(E->getType()); } +static llvm::SmallDenseMap<int, int> createRelativeIntSizesMap() { + llvm::SmallDenseMap<int, int> Result(14); + Result[BuiltinType::UChar] = 1; + Result[BuiltinType::SChar] = 1; + Result[BuiltinType::Char_U] = 1; + Result[BuiltinType::Char_S] = 1; + Result[BuiltinType::UShort] = 2; + Result[BuiltinType::Short] = 2; + Result[BuiltinType::UInt] = 3; + Result[BuiltinType::Int] = 3; + Result[BuiltinType::ULong] = 4; + Result[BuiltinType::Long] = 4; + Result[BuiltinType::ULongLong] = 5; + Result[BuiltinType::LongLong] = 5; + Result[BuiltinType::UInt128] = 6; + Result[BuiltinType::Int128] = 6; + return Result; +} + +static llvm::SmallDenseMap<int, int> createRelativeCharSizesMap() { + llvm::SmallDenseMap<int, int> Result(6); + Result[BuiltinType::UChar] = 1; + Result[BuiltinType::SChar] = 1; + Result[BuiltinType::Char_U] = 1; + Result[BuiltinType::Char_S] = 1; + Result[BuiltinType::Char16] = 2; + Result[BuiltinType::Char32] = 3; + return Result; +} + +static llvm::SmallDenseMap<int, int> createRelativeCharSizesWMap() { + llvm::SmallDenseMap<int, int> Result(6); + Result[BuiltinType::UChar] = 1; + Result[BuiltinType::SChar] = 1; + Result[BuiltinType::Char_U] = 1; + Result[BuiltinType::Char_S] = 1; + Result[BuiltinType::WChar_U] = 2; + Result[BuiltinType::WChar_S] = 2; + return Result; +} + +static bool isFirstWider(BuiltinType::Kind First, BuiltinType::Kind Second) { + static const llvm::SmallDenseMap<int, int> RelativeIntSizes( + createRelativeIntSizesMap()); + static const llvm::SmallDenseMap<int, int> RelativeCharSizes( + createRelativeCharSizesMap()); + static const llvm::SmallDenseMap<int, int> RelativeCharSizesW( + createRelativeCharSizesWMap()); + + int FirstSize, SecondSize; + if ((FirstSize = RelativeIntSizes.lookup(First)) && + (SecondSize = RelativeIntSizes.lookup(Second))) + return FirstSize > SecondSize; + if ((FirstSize = RelativeCharSizes.lookup(First)) && + (SecondSize = RelativeCharSizes.lookup(Second))) + return FirstSize > SecondSize; + if ((FirstSize = RelativeCharSizesW.lookup(First)) && + (SecondSize = RelativeCharSizesW.lookup(Second))) + return FirstSize > SecondSize; + return false; +} + void MisplacedWideningCastCheck::check(const MatchFinder::MatchResult &Result) { - const auto *Cast = Result.Nodes.getNodeAs<ExplicitCastExpr>("Cast"); + const auto *Cast = Result.Nodes.getNodeAs<CastExpr>("Cast"); + if (!CheckImplicitCasts && isa<ImplicitCastExpr>(Cast)) + return; if (Cast->getLocStart().isMacroID()) return; @@ -95,24 +178,15 @@ void MisplacedWideningCastCheck::check(const MatchFinder::MatchResult &Result) { // If CalcType and CastType have same size then there is no real danger, but // there can be a portability problem. + if (Context.getIntWidth(CastType) == Context.getIntWidth(CalcType)) { - if (CalcType->isSpecificBuiltinType(BuiltinType::Int) || - CalcType->isSpecificBuiltinType(BuiltinType::UInt)) { - // There should be a warning when casting from int to long or long long. - if (!CastType->isSpecificBuiltinType(BuiltinType::Long) && - !CastType->isSpecificBuiltinType(BuiltinType::ULong) && - !CastType->isSpecificBuiltinType(BuiltinType::LongLong) && - !CastType->isSpecificBuiltinType(BuiltinType::ULongLong)) - return; - } else if (CalcType->isSpecificBuiltinType(BuiltinType::Long) || - CalcType->isSpecificBuiltinType(BuiltinType::ULong)) { - // There should be a warning when casting from long to long long. - if (!CastType->isSpecificBuiltinType(BuiltinType::LongLong) && - !CastType->isSpecificBuiltinType(BuiltinType::ULongLong)) - return; - } else { + const auto *CastBuiltinType = + dyn_cast<BuiltinType>(CastType->getUnqualifiedDesugaredType()); + const auto *CalcBuiltinType = + dyn_cast<BuiltinType>(CalcType->getUnqualifiedDesugaredType()); + if (CastBuiltinType && CalcBuiltinType && + !isFirstWider(CastBuiltinType->getKind(), CalcBuiltinType->getKind())) return; - } } // Don't write a warning if we can easily see that the result is not diff --git a/clang-tools-extra/clang-tidy/misc/MisplacedWideningCastCheck.h b/clang-tools-extra/clang-tidy/misc/MisplacedWideningCastCheck.h index 7f08751a7d0..1c3bc4a11d9 100644 --- a/clang-tools-extra/clang-tidy/misc/MisplacedWideningCastCheck.h +++ b/clang-tools-extra/clang-tidy/misc/MisplacedWideningCastCheck.h @@ -16,19 +16,27 @@ namespace clang { namespace tidy { namespace misc { -/// Find explicit redundant casts of calculation results to bigger type. -/// Typically from int to long. If the intention of the cast is to avoid loss -/// of precision then the cast is misplaced, and there can be loss of -/// precision. Otherwise such cast is ineffective. +/// Find casts of calculation results to bigger type. Typically from int to +/// long. If the intention of the cast is to avoid loss of precision then +/// the cast is misplaced, and there can be loss of precision. Otherwise +/// such cast is ineffective. +/// +/// There is one option: +/// +/// - `CheckImplicitCasts`: Whether to check implicit casts as well which may +// be the most common case. Enabled by default. /// /// For the user-facing documentation see: /// http://clang.llvm.org/extra/clang-tidy/checks/misc-misplaced-widening-cast.html class MisplacedWideningCastCheck : public ClangTidyCheck { public: - MisplacedWideningCastCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + MisplacedWideningCastCheck(StringRef Name, ClangTidyContext *Context); + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + const bool CheckImplicitCasts; }; } // namespace misc diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-widening-cast.rst b/clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-widening-cast.rst index 648d8d10abe..824f939e2ff 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-widening-cast.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-widening-cast.rst @@ -3,10 +3,10 @@ misc-misplaced-widening-cast ============================ -This check will warn when there is a explicit redundant cast of a calculation -result to a bigger type. If the intention of the cast is to avoid loss of -precision then the cast is misplaced, and there can be loss of precision. -Otherwise the cast is ineffective. +This check will warn when there is a cast of a calculation result to a bigger +type. If the intention of the cast is to avoid loss of precision then the cast +is misplaced, and there can be loss of precision. Otherwise the cast is +ineffective. Example code:: @@ -28,6 +28,17 @@ for instance:: return (long)x * 1000; } +Implicit casts +-------------- + +Forgetting to place the cast at all is at least as dangerous and at least as +common as misplacing it. If option ``CheckImplicitCasts`` is enabled (default) +the checker also detects these cases, for instance:: + + long f(int x) { + return x * 1000; + } + Floating point -------------- diff --git a/clang-tools-extra/test/clang-tidy/misc-misplaced-widening-cast-explicit-only.cpp b/clang-tools-extra/test/clang-tidy/misc-misplaced-widening-cast-explicit-only.cpp new file mode 100644 index 00000000000..6b236a78048 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/misc-misplaced-widening-cast-explicit-only.cpp @@ -0,0 +1,58 @@ +// RUN: %check_clang_tidy %s misc-misplaced-widening-cast %t -- -config="{CheckOptions: [{key: misc-misplaced-widening-cast.CheckImplicitCasts, value: 0}]}" -- + +void func(long arg) {} + +void assign(int a, int b) { + long l; + + l = a * b; + l = (long)(a * b); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long' is ineffective, or there is loss of precision before the conversion [misc-misplaced-widening-cast] + l = (long)a * b; + + l = a << 8; + l = (long)(a << 8); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long' + l = (long)b << 8; + + l = static_cast<long>(a * b); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long' +} + +void compare(int a, int b, long c) { + bool l; + + l = a * b == c; + l = c == a * b; + l = (long)(a * b) == c; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long' + l = c == (long)(a * b); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: either cast from 'int' to 'long' + l = (long)a * b == c; + l = c == (long)a * b; +} + +void init(unsigned int n) { + long l1 = n << 8; + long l2 = (long)(n << 8); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: either cast from 'unsigned int' to 'long' + long l3 = (long)n << 8; +} + +void call(unsigned int n) { + func(n << 8); + func((long)(n << 8)); + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: either cast from 'unsigned int' to 'long' + func((long)n << 8); +} + +long ret(int a) { + if (a < 0) { + return a * 1000; + } else if (a > 0) { + return (long)(a * 1000); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: either cast from 'int' to 'long' + } else { + return (long)a * 1000; + } +} diff --git a/clang-tools-extra/test/clang-tidy/misc-misplaced-widening-cast.cpp b/clang-tools-extra/test/clang-tidy/misc-misplaced-widening-cast.cpp index 68db857c704..9e7cd8134d4 100644 --- a/clang-tools-extra/test/clang-tidy/misc-misplaced-widening-cast.cpp +++ b/clang-tools-extra/test/clang-tidy/misc-misplaced-widening-cast.cpp @@ -1,29 +1,67 @@ -// RUN: %check_clang_tidy %s misc-misplaced-widening-cast %t +// RUN: %check_clang_tidy %s misc-misplaced-widening-cast %t -- -config="{CheckOptions: [{key: misc-misplaced-widening-cast.CheckImplicitCasts, value: 1}]}" -- + +void func(long arg) {} void assign(int a, int b) { long l; l = a * b; - l = (long)(a * b); // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long' is ineffective, or there is loss of precision before the conversion [misc-misplaced-widening-cast] + l = (long)(a * b); + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long' l = (long)a * b; + l = a << 8; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long' l = (long)(a << 8); // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long' l = (long)b << 8; l = static_cast<long>(a * b); - // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long' is ineffective, or there is loss of precision before the conversion [misc-misplaced-widening-cast] + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long' +} + +void compare(int a, int b, long c) { + bool l; + + l = a * b == c; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long' + l = c == a * b; + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: either cast from 'int' to 'long' + l = (long)(a * b) == c; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long' + l = c == (long)(a * b); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: either cast from 'int' to 'long' + l = (long)a * b == c; + l = c == (long)a * b; } void init(unsigned int n) { - long l = (long)(n << 8); - // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: either cast from 'unsigned int' + long l1 = n << 8; + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: either cast from 'unsigned int' to 'long' + long l2 = (long)(n << 8); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: either cast from 'unsigned int' to 'long' + long l3 = (long)n << 8; +} + +void call(unsigned int n) { + func(n << 8); + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: either cast from 'unsigned int' to 'long' + func((long)(n << 8)); + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: either cast from 'unsigned int' to 'long' + func((long)n << 8); } long ret(int a) { - return (long)(a * 1000); - // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: either cast from 'int' to 'long' + if (a < 0) { + return a * 1000; + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: either cast from 'int' to 'long' + } else if (a > 0) { + return (long)(a * 1000); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: either cast from 'int' to 'long' + } else { + return (long)a * 1000; + } } void dontwarn1(unsigned char a, int i, unsigned char *p) { @@ -33,9 +71,9 @@ void dontwarn1(unsigned char a, int i, unsigned char *p) { // The result is a 12 bit value, there is no truncation in the implicit cast. l = (long)(a << 4); // The result is a 3 bit value, there is no truncation in the implicit cast. - l = (long)((i%5)+1); + l = (long)((i % 5) + 1); // The result is a 16 bit value, there is no truncation in the implicit cast. - l = (long)(((*p)<<8) + *(p+1)); + l = (long)(((*p) << 8) + *(p + 1)); } template <class T> struct DontWarn2 { |
