[PATCH] D93822: [clang-tidy] Add check for implicit widening of multiplication result

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added a comment.

The CI is showing build failures and there are some clang-tidy nits to be addressed as well.



================
Comment at: clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:29
+  // Is this:  long r = int(x) * int(y);  ?
+  // FIXME: shall we skip brackets/casts/etc?
+  auto *BO = dyn_cast<BinaryOperator>(E);
----------------
I think we should skip parens at the very least. `x * y` should check the same as `(x) * (y)`.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:32
+  if (!BO || BO->getOpcode() != BO_Mul)
+    // FIXME: what about:  long r = int(x) + (int(y) * int(z));  ?
+    return nullptr;
----------------
I think that could be handled in a follow-up if we find the need, WDYT?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:75
+  else if (Ty->isSignedIntegerType()) {
+    assert(ETy->isUnsignedIntegerType() &&
+           "Expected source type to be signed.");
----------------
Might be worth it to have tests around `signed char`, `unsigned char`, and `char` explicitly, as that gets awkward.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:126
+  QualType SizeTy = IndexExprType->isSignedIntegerType() ? SSizeTy : USizeTy;
+  // FIXME: is there a way to actually get the QualType for size_t/ssize_t?
+  StringRef TyAsString =
----------------
You already are using the way?


================
Comment at: clang/lib/AST/ASTContext.cpp:10158
+  if (const auto *ETy = T->getAs<EnumType>())
+    T = ETy->getDecl()->getIntegerType();
+
----------------
This looks to be getting the same value as in the `getCorrespondingUnsignedType()` call?


================
Comment at: clang/lib/AST/ASTContext.cpp:10204
+    return SatLongFractTy;
+  default:
+    llvm_unreachable("Unexpected unsigned integer or fixed point type");
----------------
It looks like `_ExtInt` is missing from both this function and the corresponding unsigned one.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93822/new/

https://reviews.llvm.org/D93822