[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

133

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

154–155

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

154–155

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

116

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.

154–155

Looks much better now, thanks!

155

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

160

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

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

1

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

[baloghadamsoftware]

Requested revision done.

[baloghadamsoftware]

clang-tidy/misc/MisplacedWideningCastCheck.cpp

116

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

160

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

116

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

160

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

 #include "clang/ASTMatchers/ASTMatchFinder.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 bool isFirstWider(BuiltinType::Kind First, BuiltinType::Kind Second) {
+  static const std::map<BuiltinType::Kind, int> 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<BuiltinType::Kind, int> 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<BuiltinType::Kind, int> 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<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 {
+    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
   // truncated.
   if (Context.getIntWidth(CalcType) >= getMaxCalculationWidth(Context, Calc))
     return;

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.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);
   }
 };