[PATCH] D53488: [clang-tidy] Improving narrowing conversions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:17
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/Mutex.h"
+
----------------
Is this include needed?


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:60
+static const BuiltinType *getBuiltinType(const Expr *E) {
+  if (const Type *T = E->getType().getCanonicalType().getTypePtrOrNull())
+    return T->getAs<BuiltinType>();
----------------
Is there a case where you expect the QualType to be invalid? If not, drop the `OrNull` bit.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:69-70
+  bool Contains(const IntegerRange &From) const {
+    return (llvm::APSInt::compareValues(Lower, From.Lower) <= 0) &&
+           (llvm::APSInt::compareValues(Upper, From.Upper) >= 0);
+  }
----------------
Spurious parens.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:74
+  bool Contains(const llvm::APSInt &Value) const {
+    return (llvm::APSInt::compareValues(Lower, Value) <= 0) &&
+           (llvm::APSInt::compareValues(Upper, Value) >= 0);
----------------
Spurious parens.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:86
+                                   const BuiltinType *T) {
+  assert(T && "T must not be nullptr");
+  if (T->isFloatingPoint()) {
----------------
Then T should be passed in as a reference type instead of using this assertion.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:109
+            llvm::APSInt::getMaxValue(Context.getTypeSize(T), false)};
+  if (T->isUnsignedInteger())
+    return {llvm::APSInt::getMinValue(Context.getTypeSize(T), true),
----------------
Perhaps a better approach is to assert this is true and then unilaterally return.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:113-114
+
+  llvm::errs() << "Unhandled type " << T->getName(Context.getPrintingPolicy())
+               << "\n";
+  llvm_unreachable("Unhandled type");
----------------
This seems like debugging code that should be removed.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:133
+
+static QualType getUnqualifiedType(const Expr *Expr) {
+  return Expr->getType().getUnqualifiedType();
----------------
Please rename the identifier to not conflict with a type name.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:175-176
+    // https://clang.llvm.org/extra/clang-tidy/checks/readability-implicit-bool-conversion.html
+    if ((ToType->getKind() == BuiltinType::Bool) ||
+        (FromType->getKind() == BuiltinType::Bool))
       return;
----------------
Spurious parens.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:208-210
+  llvm::errs() << "Unhandled from type "
+               << FromType->getName(Context.getPrintingPolicy()) << "or to type"
+               << ToType->getName(Context.getPrintingPolicy()) << "\n";
----------------
Debugging code. Assert above and unilaterally return.


================
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:309
+                          Cast->getExprLoc(), Cast, Cast->getSubExpr());
+  llvm_unreachable("must be binary operator or cast expression");
 }
----------------
Assert the above condition and unconditionally return.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53488