Index: clang-tidy/misc/BoolPointerImplicitConversionCheck.cpp =================================================================== --- clang-tidy/misc/BoolPointerImplicitConversionCheck.cpp +++ clang-tidy/misc/BoolPointerImplicitConversionCheck.cpp @@ -61,7 +61,8 @@ *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()) Index: clang-tidy/misc/MisplacedWideningCastCheck.h =================================================================== --- clang-tidy/misc/MisplacedWideningCastCheck.h +++ clang-tidy/misc/MisplacedWideningCastCheck.h @@ -16,19 +16,27 @@ 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 Index: clang-tidy/misc/MisplacedWideningCastCheck.cpp =================================================================== --- clang-tidy/misc/MisplacedWideningCastCheck.cpp +++ clang-tidy/misc/MisplacedWideningCastCheck.cpp @@ -17,6 +17,16 @@ 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 +35,22 @@ 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 +93,45 @@ return Context.getIntWidth(E->getType()); } +static bool isFirstWider(BuiltinType::Kind First, BuiltinType::Kind Second) { + static const std::map RelativeIntSizes = { + {BuiltinType::UChar, 1}, {BuiltinType::SChar, 1}, + {BuiltinType::Char_U, 1}, {BuiltinType::Char_S, 1}, + {BuiltinType::UShort, 2}, {BuiltinType::Short, 2}, + {BuiltinType::UInt, 3}, {BuiltinType::Int, 3}, + {BuiltinType::ULong, 4}, {BuiltinType::Long, 4}, + {BuiltinType::ULongLong, 5}, {BuiltinType::LongLong, 5}, + {BuiltinType::UInt128, 6}, {BuiltinType::Int128, 6}}; + + static const std::map RelativeCharSizes = { + {BuiltinType::UChar, 1}, {BuiltinType::SChar, 1}, + {BuiltinType::Char_U, 1}, {BuiltinType::Char_S, 1}, + {BuiltinType::Char16, 2}, {BuiltinType::Char32, 3}}; + + static const std::map RelativeCharSizesW = { + {BuiltinType::UChar, 1}, {BuiltinType::SChar, 1}, + {BuiltinType::Char_U, 1}, {BuiltinType::Char_S, 1}, + {BuiltinType::WChar_U, 2}, {BuiltinType::WChar_S, 2}}; + + if (RelativeIntSizes.find(First) != RelativeIntSizes.end() && + RelativeIntSizes.find(Second) != RelativeIntSizes.end()) { + return RelativeIntSizes.at(First) > RelativeIntSizes.at(Second); + } + if (RelativeCharSizes.find(First) != RelativeCharSizes.end() && + RelativeCharSizes.find(Second) != RelativeCharSizes.end()) { + return RelativeCharSizes.at(First) > RelativeCharSizes.at(Second); + } + if (RelativeCharSizesW.find(First) != RelativeCharSizesW.end() && + RelativeCharSizesW.find(Second) != RelativeCharSizesW.end()) { + return RelativeCharSizesW.at(First) > RelativeCharSizesW.at(Second); + } + return false; +} + void MisplacedWideningCastCheck::check(const MatchFinder::MatchResult &Result) { - const auto *Cast = Result.Nodes.getNodeAs("Cast"); + const auto *Cast = Result.Nodes.getNodeAs("Cast"); + if (!CheckImplicitCasts && isa(Cast)) + return; if (Cast->getLocStart().isMacroID()) return; @@ -95,22 +150,14 @@ // 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(CastType->getUnqualifiedDesugaredType()); + const auto CalcBuiltinType = + dyn_cast(CalcType->getUnqualifiedDesugaredType()); + if (CastBuiltinType && CalcBuiltinType && + !isFirstWider(CastBuiltinType->getKind(), CalcBuiltinType->getKind())) { return; } } Index: docs/clang-tidy/checks/misc-misplaced-widening-cast.rst =================================================================== --- docs/clang-tidy/checks/misc-misplaced-widening-cast.rst +++ 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 @@ 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 -------------- Index: test/clang-tidy/misc-misplaced-widening-cast.cpp =================================================================== --- test/clang-tidy/misc-misplaced-widening-cast.cpp +++ 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(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 @@ // 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 struct DontWarn2 {