aaron.ballman added inline comments.
================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:114
@@ -112,1 +113,3 @@
+// Perform a comparison bitween APSInt with respect to bit-width and
signedness.
+static int compareValues(const llvm::APSInt &ValueLHS,
----------------
s/bitween/between, however, I don't think this function adds much value; why
not just call `APSInt::compareValues()` directly?
================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:131
@@ +130,3 @@
+
+ // Handle the case where constants are off by one: x <= 4 <==> x < 5.
+ llvm::APSInt ValueLHS_plus1 = ValueLHS;
----------------
Why is off-by-one more useful than a stronger range analysis?
================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:171
@@ +170,3 @@
+ ++ValueLHS_plus1;
+ if (compareValues(ValueLHS_plus1, ValueRHS) == 0 && OpcodeLHS == BO_GT &&
+ OpcodeRHS == BO_LT) {
----------------
Same question here as above.
================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:177
@@ +176,3 @@
+ // Handle cases where the constants are different.
+ if ((OpcodeLHS == BO_EQ || OpcodeLHS == BO_LE || OpcodeLHS == BO_LE) &&
+ (OpcodeRHS == BO_EQ || OpcodeRHS == BO_GT || OpcodeRHS == BO_GE))
----------------
Can this be done before doing the more expensive value comparisons?
================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:227
@@ +226,3 @@
+
+static bool doRangeSubsumeRange(BinaryOperatorKind OpcodeLHS,
+ const llvm::APSInt &ValueLHS,
----------------
s/do/does
================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:267
@@ +266,3 @@
+ Opcode = BO_Add;
+ Value = -Value;
+ }
----------------
I can never remember myself, but how well does APSInt handle this situation if
it causes overflow of the signed value? e.g., an 8-bit APSInt holding the value
-128 being negated to 128 (which is outside the range of an 8-bit signed
integer).
================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:294
@@ +293,3 @@
+// Returns a matcher for a generic expression (not a constant expression).
+static ast_matchers::internal::Matcher<Expr> matchGenericExpr(StringRef Id) {
+ std::string SymId = (Id + "-gen").str();
----------------
I'm not keen on this name because C11 has `_Generic`, for which we have a
`GenericSelectionExpr` which is awfully close to this name.
================
Comment at: clang-tidy/misc/RedundantExpressionCheck.h:31
@@ +30,3 @@
+private:
+ void checkArithmeticExpr(const ast_matchers::MatchFinder::MatchResult & R);
+ void checkBitwiseExpr(const ast_matchers::MatchFinder::MatchResult &R);
----------------
`&` should bind to `R`.
Also, all of these functions can be marked `const`.
http://reviews.llvm.org/D21392
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
