erichkeane added inline comments.
================
Comment at: lib/CodeGen/CGExprScalar.cpp:1008
+ // We ignore conversions to/from pointer and/or bool.
+ if (!(SrcType->isIntegerType() && DstType->isIntegerType()))
+ return;
----------------
I'd rather !SrcType->isInt || !DestType->isInt
================
Comment at: lib/CodeGen/CGExprScalar.cpp:1011
+
+ assert(isa<llvm::IntegerType>(SrcTy) && isa<llvm::IntegerType>(DstTy) &&
+ "clang integer type lowered to non-integer llvm type");
----------------
This seems like a silly assert, since you did the check above.
================
Comment at: lib/CodeGen/CGExprScalar.cpp:1030
+ {
+ // At least one of the values needs to have signed type.
+ // If both are unsigned, then obviously, neither of them can be negative.
----------------
Does this really need its own scope?
================
Comment at: lib/CodeGen/CGExprScalar.cpp:1032
+ // If both are unsigned, then obviously, neither of them can be negative.
+ if (!(SrcSigned || DstSigned))
+ return;
----------------
Again, I'd rather we distribute the '!'.
================
Comment at: lib/CodeGen/CGExprScalar.cpp:1035
+ }
+ {
+ // If the conversion is to *larger* *signed* type, then no check is needed.
----------------
These scopes are getting out of hand... just kill them all. Introducing
CanonSrcType/CanonDstType into the larger scope isn't that big of a deal.
================
Comment at: lib/CodeGen/CGExprScalar.cpp:1044
+
+ assert(!DstType->isBooleanType() && "we should not get here with booleans.");
+
----------------
Curious what prevents this?
================
Comment at: lib/CodeGen/CGExprScalar.cpp:1054
+ const char *Name) -> Value * {
+ // Does this Value has signed type?
+ bool VSigned = VType->isSignedIntegerOrEnumerationType();
----------------
// Is this value a signed type?
================
Comment at: lib/CodeGen/CGExprScalar.cpp:2004
+ if (auto *ICE = dyn_cast<ImplicitCastExpr>(CE)) {
+ if (CGF.SanOpts.hasOneOf(SanitizerKind::ImplicitConversion) &&
+ !ICE->isPartOfExplicitCast()) {
----------------
Is this an error? You swapped a 'has' with a 'hasOneOf' but only listed a
single thing.
Repository:
rC Clang
https://reviews.llvm.org/D50250
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
