aaron.ballman added a comment.

The CI is showing build failures and there are some clang-tidy nits to be 
addressed as well.



================
Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:29
+  // Is this:  long r = int(x) * int(y);  ?
+  // FIXME: shall we skip brackets/casts/etc?
+  auto *BO = dyn_cast<BinaryOperator>(E);
----------------
I think we should skip parens at the very least. `x * y` should check the same 
as `(x) * (y)`.


================
Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:32
+  if (!BO || BO->getOpcode() != BO_Mul)
+    // FIXME: what about:  long r = int(x) + (int(y) * int(z));  ?
+    return nullptr;
----------------
I think that could be handled in a follow-up if we find the need, WDYT?


================
Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:75
+  else if (Ty->isSignedIntegerType()) {
+    assert(ETy->isUnsignedIntegerType() &&
+           "Expected source type to be signed.");
----------------
Might be worth it to have tests around `signed char`, `unsigned char`, and 
`char` explicitly, as that gets awkward.


================
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 =
----------------
You already are using the way?


================
Comment at: clang/lib/AST/ASTContext.cpp:10158
+  if (const auto *ETy = T->getAs<EnumType>())
+    T = ETy->getDecl()->getIntegerType();
+
----------------
This looks to be getting the same value as in the 
`getCorrespondingUnsignedType()` call?


================
Comment at: clang/lib/AST/ASTContext.cpp:10204
+    return SatLongFractTy;
+  default:
+    llvm_unreachable("Unexpected unsigned integer or fixed point type");
----------------
It looks like `_ExtInt` is missing from both this function and the 
corresponding unsigned one.


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

Reply via email to