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

[baloghadamsoftware]

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.

[baloghadamsoftware]

Prerequisites (matchers) are accepted now.

[alexfh]

clang-tidy/misc/BoolPointerImplicitConversionCheck.cpp

64–65

The line exceeds 80 columns limit. Please clang-format the change.

clang-tidy/misc/MisplacedWideningCastCheck.cpp

161

isa<T> should be used instead of dyn_cast<T>, when you don't need the resulting pointer.

182–183

isSpecificBuiltinType is not the best tool here. Instead, we could get the builtin type kinds and work with them (CalcType->getAs<BuiltinType>()->getKind() and CastType->getAs<BuiltinType>()->getKind() after checking that both are BuiltinTypes).

test/clang-tidy/misc-misplaced-widening-cast.cpp

28

Please truncate "is ineffective, or there is loss of precision before the conversion [misc-misplaced-widening-cast]" in all CHECK-MESSAGES lines but the first one.

41

It makes sense to leave the to '<target type' part in all messages.

[baloghadamsoftware]

clang-tidy/misc/MisplacedWideningCastCheck.cpp

182–183

I do not like it either, just reused the original code and copy-pasted it. I also think that getKind() should be used and instead of the long branches we could do the comparison using relative size table(s).

[baloghadamsoftware]

Required fixes done.

[alexfh]

clang-tidy/misc/MisplacedWideningCastCheck.cpp

117

This code shouldn't repeat the lookups. You can, for example, use iterators to keep the results of .find().

It also seems more appropriate to use SmallDenseMap.

182–188

Looks much better now, thanks!

183–188

Please use const auto * to make it obvious it's a pointer.

188

No need for the braces around single-line if bodies.

test/clang-tidy/misc-misplaced-widening-cast.cpp

1–3

We should also have a test with this option turned off.

[baloghadamsoftware]

Requested revision done.

[baloghadamsoftware]

clang-tidy/misc/MisplacedWideningCastCheck.cpp

117

I changed to SmallDenseMap and its lookup() member function is nice. However it unfortunately lacks an initializer_list constructor.

188

Is this an LLVM style rule? I always learned that it is the safest to use braces even for single-line if bodies.

[alexfh]

Looks good! Thank you for addressing the comments.

clang-tidy/misc/MisplacedWideningCastCheck.cpp

117

It has a constructor from two iterators, but I'm not sure it'll result in a significantly better code.

188

http://llvm.org/docs/CodingStandards.html is surprisingly inconsistent in this regard, but "no braces around single-line if/for/... bodies" is a more common style in LLVM and, in particular, in clang-tidy code. The problems this style might lead to (statements indented as-if they were under if/for/..., but they actually aren't) are mitigated by the wide use of clang-format.

clang-tidy/misc/BoolPointerImplicitConversionCheck.cpp

void BoolPointerImplicitConversionCheck::check(
   auto DeclRef = ignoringParenImpCasts(declRefExpr(to(equalsNode(D))));
   if (!match(findAll(
                  unaryOperator(hasOperatorName("*"), hasUnaryOperand(DeclRef))),
              *If, *Result.Context).empty() ||
       !match(findAll(arraySubscriptExpr(hasBase(DeclRef))), *If,
              *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())
     return;
 
   diag(Var->getLocStart(), "dubious check of 'bool *' against 'nullptr', did "
                            "you mean to dereference it?")
       << FixItHint::CreateInsertion(Var->getLocStart(), "*");
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

clang-tidy/misc/MisplacedWideningCastCheck.h

 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MISPLACED_WIDENING_CAST_H
 
 #include "../ClangTidy.h"
 
 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
 } // namespace tidy
 } // namespace clang
 
 #endif

clang-tidy/misc/MisplacedWideningCastCheck.cpp

 //===--- MisplacedWideningCastCheck.cpp - clang-tidy-----------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "MisplacedWideningCastCheck.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/DenseMap.h"
 
 using namespace clang::ast_matchers;
 
 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("-"),
                              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) {
   E = E->IgnoreParenImpCasts();
 
   if (const auto *Bop = dyn_cast<BinaryOperator>(E)) {
     unsigned LHSWidth = getMaxCalculationWidth(Context, Bop->getLHS());
     unsigned RHSWidth = getMaxCalculationWidth(Context, Bop->getRHS());
if (const auto *Bop = dyn_cast<BinaryOperator>(E)) {
     return T->isIntegerType() ? Context.getIntWidth(T) : 1024U;
   } else if (const auto *I = dyn_cast<IntegerLiteral>(E)) {
     return I->getValue().getActiveBits();
   }
 
   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;
 
   const auto *Calc = Result.Nodes.getNodeAs<Expr>("Calc");
   if (Calc->getLocStart().isMacroID())
     return;
 
   ASTContext &Context = *Result.Context;
 
   QualType CastType = Cast->getType();
   QualType CalcType = Calc->getType();
 
   // Explicit truncation using cast.
   if (Context.getIntWidth(CastType) < Context.getIntWidth(CalcType))
     return;
 
   // 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 {
-      return;
-    }
+    const auto *CastBuiltinType =
+        dyn_cast<BuiltinType>(CastType->getUnqualifiedDesugaredType());
+    const auto *CalcBuiltinType =
+        dyn_cast<BuiltinType>(CalcType->getUnqualifiedDesugaredType());
+    if (CastBuiltinType && CalcBuiltinType &&
+        !isFirstWider(CastBuiltinType->getKind(), CalcBuiltinType->getKind()))
   }
 
   // Don't write a warning if we can easily see that the result is not
   // truncated.
   if (Context.getIntWidth(CalcType) >= getMaxCalculationWidth(Context, Calc))
     return;
 
   diag(Cast->getLocStart(), "either cast from %0 to %1 is ineffective, or "
                             "there is loss of precision before the conversion")
       << CalcType << CastType;
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

docs/clang-tidy/checks/misc-misplaced-widening-cast.rst

 .. title:: clang-tidy - misc-misplaced-widening-cast
 
 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::
 
     long f(int x) {
         return (long)(x*1000);
     }
 
 The result x*1000 is first calculated using int precision. If the result
 exceeds int precision there is loss of precision. Then the result is casted to
 long.
 
 If there is no loss of precision then the cast can be removed or you can
 explicitly cast to int instead.
 
 If you want to avoid loss of precision then put the cast in a proper location,
 for instance::
 
     long f(int x) {
         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
 --------------
 
 Currently warnings are only written for integer conversion. No warning is
 written for this code::
 
     double f(float x) {
         return (double)(x * 10.0f);
     }

test/clang-tidy/misc-misplaced-widening-cast-explicit-only.cpp

+// 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;
+  }
+}

test/clang-tidy/misc-misplaced-widening-cast.cpp

-// 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) {
+  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]]:10: warning: either cast from 'int' to 'long'
+    // 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) {
   long l;
   // The result is a 9 bit value, there is no truncation in the implicit cast.
   l = (long)(a + 15);
   // 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 {
   void assign(T a, T b) {
     long l;
     l = (long)(a * b);
   }
 };