This patch is adding support to recognize more complex redundant expressions.
Details
Diff Detail
Event Timeline
clang-tidy/misc/RedundantExpressionCheck.cpp | ||
---|---|---|
124 | s/bitween/between, however, I don't think this function adds much value; why not just call APSInt::compareValues() directly? | |
141 | Why is off-by-one more useful than a stronger range analysis? | |
181 | Same question here as above. | |
187 | Can this be done before doing the more expensive value comparisons? | |
237 | s/do/does | |
277 | 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). | |
304 | I'm not keen on this name because C11 has _Generic, for which we have a GenericSelectionExpr which is awfully close to this name. | |
clang-tidy/misc/RedundantExpressionCheck.h | ||
31 | & should bind to R. Also, all of these functions can be marked const. |
thx Aaron.
clang-tidy/misc/RedundantExpressionCheck.cpp | ||
---|---|---|
141 | I don't get your point? This function is comparing two ranges. Ranges are comparable only if they are compared to the same constant.... 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? | |
187 | it can be done before the previous if. But not the one at line 149 (which always returns). | |
277 | In this case -128 (8-bits) will give -128. I added an unittest to cover this case. | |
304 | 'Generic' is a too generic name :) | |
clang-tidy/misc/RedundantExpressionCheck.h | ||
31 | 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 |
clang-tidy/misc/RedundantExpressionCheck.cpp | ||
---|---|---|
31 | I think this could be implemented using APInt overflow checks, no? APInt::sadd_ov()? | |
141 | Ah, this may be confusion on my part. I was thinking "equivalent ranges" meaning "does one range cover another range", e.g., x < 12 is also covered by x < 4 in an expression like if (x < 4 && x < 12). I think this code is fine as-is. | |
277 |
So negating -128 doesn't yield 128, it yields -128? That seems weird.
A real value of the same width and signedness has UB with that case, which is why I was asking. The range of an 8-bit signed int is -128 to 127, so negating -128 yields an out-of-range value. I want to make sure we aren't butchering that by canonicalizing the negate expression. | |
clang-tidy/misc/RedundantExpressionCheck.h | ||
31 | Hmmm, good point. Drat! |
A few nits.
clang-tidy/misc/RedundantExpressionCheck.cpp | ||
---|---|---|
138 | How about replacing if (x) return true; return false; with return x;? BTW, next function looks fine in this regard, since it has a chain of if (x) return true; if (y) return true; .... | |
185 | Remove braces for consistency. | |
194 | I'd slightly prefer it without "do": rangesFullyCoverDomain, rangeSubsumesRange, etc. | |
269 | The last return seems to be dead code. | |
367 | Not sure I understand this comment. | |
627 | using llvm::APSInt; to remove some boilerplate? | |
650 | Please add braces, when the body is more than one line. | |
654 | ComparisonOperator->getOperatorLoc() and the message are repeated too many times. Pull them to local variables / constants? |
clang-tidy/misc/RedundantExpressionCheck.cpp | ||
---|---|---|
31 | It could be implemented using sadd_ov and uadd_ov. Using the operator++ here let the instantiated type do the right increment and right comparison. | |
277 | The value produced by 'canonicalNegateExpr' is the same value produced by executing the sub instruction on the CPU. Btw, there is a unittest to cover this case. | |
367 | if ( implicit-int-to-bool(x) ) <<-- the implicit-int-to-bool(...) could be consider as x != 0 |
& should bind to R.
Also, all of these functions can be marked const.