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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
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);
----------------
aaron.ballman wrote:
> I think we should skip parens at the very least. `x * y` should check the same as `(x) * (y)`.
This is more about the future case for which there's FIXME few lines down.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:75
+  else if (Ty->isSignedIntegerType()) {
+    assert(ETy->isUnsignedIntegerType() &&
+           "Expected source type to be signed.");
----------------
lebedev.ri wrote:
> aaron.ballman wrote:
> > Might be worth it to have tests around `signed char`, `unsigned char`, and `char` explicitly, as that gets awkward.
> Are you asking for test coverage, or for explicit handling of those types?
> I'll add tests.
It seems to kinda just work, because of the promotion to `int`.


================
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 =
----------------
aaron.ballman wrote:
> You already are using the way?
No, that's not it, because `SizeTy.getAsString()` will not be `size_t`/`ssize_t`, but `long`/etc.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-char.cpp:10
+
+long t0(char a, char b) {
+  return a * b;
----------------
aaron.ballman wrote:
> Can you also test the fixit behavior in addition to the diagnostic behavior (at least one test for each kind of fix-it)?
I would love to do that, but as we have briefly talked with @njames93,
fix-it testing in clang-tidy, as compared to clang proper,
is rudimentary. I basically can not add tests here.
As far as i can see, it doesn't want to apply fix-its, because there are two of them.

So this is the best i can do, take it or leave it. /s


================
Comment at: clang/lib/AST/ASTContext.cpp:10158
+  if (const auto *ETy = T->getAs<EnumType>())
+    T = ETy->getDecl()->getIntegerType();
+
----------------
lebedev.ri wrote:
> aaron.ballman wrote:
> > This looks to be getting the same value as in the `getCorrespondingUnsignedType()` call?
> Yep. Are both of them incorrect?
> I'm not sure what should be returned here.
Actually, this is correct.
Note that we don't return it, but the later code will flip it's sign.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93822