[PATCH] D55125: [clang-tidy] Fix a false positive in misc-redundant-expression check

[Daniel Krupp via Phabricator via cfe-commits]

dkrupp added a comment.

In D55125#1315335 <https://reviews.llvm.org/D55125#1315335>, @Szelethus wrote:

> I see your point, but here's why I think it isn't a bug: I like to see macros as `constexpr` variables, and if I used those instead, I personally wouldn't like to get a warning just because they have the same value. In C, silencing such a warning isn't even really possible.
>
> Another interesting thought, @donat.nagy's check works by comparing actual nodes of the AST, while this one would work with `Lexer`, but they almost want to do the same thing, the only difference is that the first checks whether two pieces of code are equivalent, and the second checks whether they are the same. Maybe it'd be worth to extract the logic into a simple `areStmtsEqual(const Stmt *S1, const Stmt *S2, bool ShouldCompareLexically = false)` function, that would do AST based comparison if the last param is set to false, and would use `Lexer` if set to true. After that, we could just add command line options to both of these checks, to leave it up to the user to choose in between them. Maybe folks who suffer from heavily macro-infested code would rather bear the obvious performance hit `Lexer` causes for little more precise warnings, and (for example) users of C++11 (because there are few excuses not to prefer `constexpr` there) could run in AST mode.
>
> edit: I'm not actually all that sure about the performance hit. Just a guess.
>
> But I'm just thinking aloud really.


Wiring out the lexical comparison and AST based comparison sounds like an interesting idea, however IMHO such a setting is too coarse-grained on the file level.  My guess would be that it depends from expression (fragment) to expression fragment how you want to compare them: for macros lexical comparison is better, for arithmetic expressions with many parentheses AST based recursive comparison may be a better fit (as implemented also in this checker).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55125/new/

https://reviews.llvm.org/D55125