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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
