[PATCH] D38688: [clang-tidy] Misc redundant expressions checker updated for macros

[Aaron Ballman via Phabricator via cfe-commits]

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