baloghadamsoftware created this revision.
baloghadamsoftware added a reviewer: alexfh.
baloghadamsoftware added a subscriber: cfe-commits.
Existing checker misc-misplaced-widening-cast was extended:
- New use cases: casted expression as lhs or rhs of a logical comparison or
function argument
- New types: beside int, long and long long various char types, short and
int128 added
- New option to check implicit casts: forgetting a cast is at least as common
and as dangerous as misplacing it. This option can be disabled.
This patch depends on AST Matcher patch D17986 and also contains fix for
checker misc-bool-pointer-implicit-conversion needed because of the fix in the
AST Matcher patch.
http://reviews.llvm.org/D17987
Files:
clang-tidy/misc/BoolPointerImplicitConversionCheck.cpp
clang-tidy/misc/MisplacedWideningCastCheck.cpp
clang-tidy/misc/MisplacedWideningCastCheck.h
docs/clang-tidy/checks/misc-misplaced-widening-cast.rst
test/clang-tidy/misc-misplaced-widening-cast.cpp
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;
+ // 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' is ineffective, or there is loss of precision before the conversion [misc-misplaced-widening-cast]
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'
+}
+
+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' is ineffective, or there is loss of precision before the conversion [misc-misplaced-widening-cast]
+ l = c == a * b;
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: 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) == c;
// 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 = c == (long)(a * b);
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: 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 == 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'
+ long l2 = (long)(n << 8);
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: either cast from 'unsigned int'
+ long l3 = (long)n << 8;
+}
+
+void call(unsigned int n) {
+ func(n << 8);
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: either cast from 'unsigned int'
+ func((long)(n << 8));
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: either cast from 'unsigned int'
+ 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 <class T> struct DontWarn2 {
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: 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,22 +17,40 @@
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("-"),
hasOperatorName("*"), hasOperatorName("<<"))),
unaryOperator(hasOperatorName("~"))),
hasType(isInteger()))
.bind("Calc");
- auto Cast = explicitCastExpr(anyOf(cStyleCastExpr(), cxxStaticCastExpr(),
- cxxReinterpretCastExpr()),
- hasDestinationType(isInteger()), has(Calc))
- .bind("Cast");
-
- Finder->addMatcher(varDecl(has(Cast)), this);
- Finder->addMatcher(returnStmt(has(Cast)), this);
+ 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(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) {
@@ -76,7 +94,9 @@
}
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 && dyn_cast<ImplicitCastExpr>(Cast))
+ return;
if (Cast->getLocStart().isMacroID())
return;
@@ -96,19 +116,69 @@
// 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 (CalcType->isSpecificBuiltinType(BuiltinType::SChar) ||
+ CalcType->isSpecificBuiltinType(BuiltinType::UChar) ||
+ CalcType->isSpecificBuiltinType(BuiltinType::Char_S) ||
+ CalcType->isSpecificBuiltinType(BuiltinType::Char_U)) {
+ // There should be a warning when casting from char to wchar_t, char16,
+ // char32, short, int, long, long long or int128
+ if (!CastType->isSpecificBuiltinType(BuiltinType::WChar_U) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::WChar_S) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::Char16) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::Char32) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::Short) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::UShort) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::Int) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::UInt) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::Long) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::ULong) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::LongLong) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::ULongLong) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::Int128) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::UInt128))
+ return;
+ } else if (CalcType->isSpecificBuiltinType(BuiltinType::Short) ||
+ CalcType->isSpecificBuiltinType(BuiltinType::UShort)) {
+ // There should be a warning when casting from short to int, long,
+ // long long or int128.
+ if (!CastType->isSpecificBuiltinType(BuiltinType::Int) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::UInt) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::Long) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::ULong) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::LongLong) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::ULongLong) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::Int128) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::UInt128))
+ return;
+ } else if (CalcType->isSpecificBuiltinType(BuiltinType::Int) ||
+ CalcType->isSpecificBuiltinType(BuiltinType::UInt)) {
+ // There should be a warning when casting from int to long, long long or
+ // int128
if (!CastType->isSpecificBuiltinType(BuiltinType::Long) &&
!CastType->isSpecificBuiltinType(BuiltinType::ULong) &&
!CastType->isSpecificBuiltinType(BuiltinType::LongLong) &&
- !CastType->isSpecificBuiltinType(BuiltinType::ULongLong))
+ !CastType->isSpecificBuiltinType(BuiltinType::ULongLong) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::Int128) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::UInt128))
return;
} else if (CalcType->isSpecificBuiltinType(BuiltinType::Long) ||
CalcType->isSpecificBuiltinType(BuiltinType::ULong)) {
- // There should be a warning when casting from long to long long.
+ // There should be a warning when casting from long to long long or
+ // int128.
if (!CastType->isSpecificBuiltinType(BuiltinType::LongLong) &&
- !CastType->isSpecificBuiltinType(BuiltinType::ULongLong))
+ !CastType->isSpecificBuiltinType(BuiltinType::ULongLong) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::Int128) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::UInt128))
+ return;
+ } else if (CalcType->isSpecificBuiltinType(BuiltinType::LongLong) ||
+ CalcType->isSpecificBuiltinType(BuiltinType::ULongLong)) {
+ // There should be a warning when casting from long long to int128.
+ if (!CastType->isSpecificBuiltinType(BuiltinType::Int128) &&
+ !CastType->isSpecificBuiltinType(BuiltinType::UInt128))
+ return;
+ } else if (CalcType->isSpecificBuiltinType(BuiltinType::Char16)) {
+ // There should be a warning when casting from char16 to char32.
+ if (!CastType->isSpecificBuiltinType(BuiltinType::Char32))
return;
} else {
return;
Index: clang-tidy/misc/BoolPointerImplicitConversionCheck.cpp
===================================================================
--- clang-tidy/misc/BoolPointerImplicitConversionCheck.cpp
+++ clang-tidy/misc/BoolPointerImplicitConversionCheck.cpp
@@ -61,7 +61,7 @@
*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())
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
