aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land.
LGTM aside from some small nits in the documentation. ================ Comment at: clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:130 + StringRef TyAsString = + IndexExprType->isSignedIntegerType() ? "ssize_t" : "size_t"; + ---------------- 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. ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-implicit-widening-of-multiplication-result.rst:40 + When suggesting fix-its for C++ code, should C++-style ``static_cast<>()``'s + be suggested, or C-style casts. + ---------------- Add info about the default behavior. ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-implicit-widening-of-multiplication-result.rst:45 + When suggesting to include the appropriate header in C++ code, + should ``<cstddef>`` header be suggested, or ``<stddef.h>``. + ---------------- Same here. 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