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