aaron.ballman added inline comments.
================ Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:633-635 +// The function discards (X < M1 && X <= M2), because it can always be +// simplified to X < M, where M is the smaller one of the two macro +// values. ---------------- barancsuk wrote: > aaron.ballman wrote: > > This worries me slightly for macros because those values can be overridden > > for different compilation targets. So the diagnostic may be correct for a > > particular configuration of the macros, but incorrect for another. Have you > > tested this against any large code bases to see what the quality of the > > diagnostics looks like? > Thank you for your insight! > > Now that I come to think of it, although these expressions are redundant for > every configuration, there is no adequate simplification for each compilation > target, because they might change their behavior if the values vary. > > A good example is the following expression, that is always redundant > regardless of the values that the macros hold. > However, the particular macro that causes the redundancy can vary from one > compilation target to another. > ``` > (X < MACRO1 || X < MACRO2) > ``` > If MACRO1 > MACRO2, the appropriate simplification is `(X < MACRO1 )`, > and if MACRO1 < MACRO2, it is `(X < MACRO2)`. > > Even expressions, like `(X < MACRO1 || X != MACRO2)`, that can almost always > be simplified to `(X != MACRO2)`, change their behavior, if MACRO1 is equal > to MACRO2 (then they are always true), and can be considered conscious > development choices. > > In conclusion, I think, it might be better now to skip these expressions, and > check for only the ones that contain the same macro twice, like `X < MACRO && > X > MACRO`. > > In conclusion, I think, it might be better now to skip these expressions, and > check for only the ones that contain the same macro twice, like X < MACRO && > X > MACRO. I think that approach makes the most sense, at least initially. https://reviews.llvm.org/D38688 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
