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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

FWIW, it looks like tests are still failing on Windows according to the CI pipeline.



================
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 =
----------------
lebedev.ri wrote:
> 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.
Ah, I see what you mean now, thanks.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:130
+  StringRef TyAsString =
+      IndexExprType->isSignedIntegerType() ? "ssize_t" : "size_t";
+
----------------
aaron.ballman wrote:
> One thing that's awkward about this is that there's no portable `ssize_t` type -- that's a POSIX type but it doesn't exist on all platforms (like Windows). We shouldn't print out a typecast that's going to cause compile errors, but we also shouldn't use the underlying type for `ssize_t` as that may be incorrect for other target architectures.
I'm still not quite certain what to do about this. Would it make sense to use the underlying type on platforms that don't have `ssize_t`? Relatedly, if we're going to suggest this as a replacement, we should also insert an include for the correct header file.


================
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;
----------------
lebedev.ri wrote:
> 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
Ah, thank you for clarifying the issue that the two fixits make it hard to test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93822