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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:67
+      << FixItHint::CreateInsertion(E->getBeginLoc(),
+                                    "(" + Ty.getAsString() + ")(")
+      << FixItHint::CreateInsertion(E->getEndLoc(), ")") << E->getSourceRange();
----------------
We might want to consider letting the user select between `static_cast` and C-style casts via an option.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:130
+  StringRef TyAsString =
+      IndexExprType->isSignedIntegerType() ? "ssize_t" : "size_t";
+
----------------
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.


================
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.
Just test coverage to make sure we're doing the right thing for them and not triggering these assertions.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-implicit-widening-of-multiplication-result.rst:6-9
+The check diagnoses instances where a result of a multiplication is implicitly
+widened, and suggests (with fix-it) to either silence the code by making
+widening explicit, or to perform the multiplication in a wider type,
+to avoid the widening afterwards.
----------------
I think the documentation would be stronger if it explained why this diagnostic is helpful (show some examples of bugs that it catches and that the suggested fixes remove the bug).


================
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;
----------------
Can you also test the fixit behavior in addition to the diagnostic behavior (at least one test for each kind of fix-it)?


================
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.
I think I confused myself on the code, I think both are correct.


================
Comment at: clang/lib/AST/ASTContext.cpp:10204
+    return SatLongFractTy;
+  default:
+    llvm_unreachable("Unexpected unsigned integer or fixed point type");
----------------
lebedev.ri wrote:
> aaron.ballman wrote:
> > It looks like `_ExtInt` is missing from both this function and the corresponding unsigned one.
> Likewise, i'm not sure what should be returned for that.
> Just the original `_ExtInt`?
`_ExtInt` has `signed` and `unsigned` variants. You can use `ASTContext::getExtIntType()` to get the type with the correct signedness. However, I see now that it's not a builtin type (huh, I thought it was!), so it'd have to be above the `switch` but after the `enum` handling.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93822