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

Tue Apr 13 11:20:55 PDT 2021

lebedev.ri added a comment.

@aaron.ballman thank you so much for the review!

================
Comment at: clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:130
+  StringRef TyAsString =
+      IndexExprType->isSignedIntegerType() ? "ssize_t" : "size_t";
+
----------------
aaron.ballman wrote:
> lebedev.ri wrote:
> > aaron.ballman wrote:
> > > 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.
> > I've been thinking about this, and i really can't come up with a better fix than using `ptrdiff_t`.
> I'm not super excited about using `ptrdiff_t` as there are not likely to be pointer subtractions in the vicinity of the code being diagnosed, but at the same time, `ptrdiff_t` is functionally the same as `size_t` except with a sign bit, which is what `ssize_t` is. I'll hold my nose for now, but if we get bug reports about this use, we may want to investigate more involved solutions.
Yep. `signed size_t` isn't a valid type, so at best i guess we could avoid emitting such a fixit.
In C++, `std::make_signed<size_t>::type` is valid, but it's kinda mouthful.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93822