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