etienneb added a comment.
thx Aaron.
================
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;
----------------
aaron.ballman wrote:
> Why is off-by-one more useful than a stronger range analysis?
I don't get your point?
This function is comparing two ranges. Ranges are comparable only if they are
compared to the same constant....
OR, if they are off-by one and the operator contains "equality".
As the comment state, x <= 4 is equivalent to x < 5.
To avoid problem with wrap around, the left value is incremented (the left
value must be the smaller one: canonical form).
I don't get why we should use a range analysis? Is there some utility functions
I'm not aware of?
================
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))
----------------
aaron.ballman wrote:
> Can this be done before doing the more expensive value comparisons?
it can be done before the previous if. But not the one at line 149 (which
always returns).
================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:267
@@ +266,3 @@
+ Opcode = BO_Add;
+ Value = -Value;
+ }
----------------
aaron.ballman wrote:
> 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).
In this case -128 (8-bits) will give -128.
The APSInt is behaving the same way than a real value of the same width and
signedness.
I added an unittest to cover this case.
================
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();
----------------
aaron.ballman wrote:
> I'm not keen on this name because C11 has `_Generic`, for which we have a
> `GenericSelectionExpr` which is awfully close to this name.
'Generic' is a too generic 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);
----------------
aaron.ballman wrote:
> `&` should bind to `R`.
>
> Also, all of these functions can be marked `const`.
They can't be constant, they are calling 'diag' which is not const.
```
error: no matching function for call to
‘clang::tidy::misc::RedundantExpressionCheck::diag(clang::SourceLocation, const
char [35]) 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
