[clang-tidy] Extension of checker misc-misplaced-widening-cast

Summary:
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 patches D17986 and D18243 and also contains fix for checker misc-bool-pointer-implicit-conversion needed because of the fix in the AST Matcher patch.

Reviewers: hokein, alexfh

Subscribers: o.gyorgy, xazax.hun, cfe-commits

Differential Revision: http://reviews.llvm.org/D17987

llvm-svn: 265532

diff --git a/clang-tools-extra/clang-tidy/misc/BoolPointerImplicitConversionCheck.cpp b/clang-tools-extra/clang-tidy/misc/BoolPointerImplicitConversionCheck.cpp
index ec13e63..7fd1ca0 100644 (file)
--- 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 a1666c5..c870da1 100644 (file)
--- 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 7f08751..1c3bc4a 100644 (file)
--- 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 648d8d1..824f939 100644 (file)
--- 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 (file)
index 0000000..6b236a7
--- /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 68db857..9e7cd81 100644 (file)
--- 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 {