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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> 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).

Yes, I agree. I think having such a utility would make sense as an future addition, but I would not like to block on it for several reasons:

- do we know how often it this case even appears?
- is it a real false positive in all cases?
- it can be silence with `// NOLINT` if it actually is a false-positive

My gut feeling is that its rather a corner-case (this late patch to misc-redundant-expression is somewhat indicative for it) and would block the patch from landing.
Allowing future room for flexibility is certainly good.



================
Comment at: test/clang-tidy/misc-redundant-expression.cpp:109
 #define COND_OP_OTHER_MACRO 9
+#define COND_OP_THIRD_MACRO COND_OP_MACRO
 int TestConditional(int x, int y) {
----------------
dkrupp wrote:
> JonasToth wrote:
> > Could you please add a test where the macro is `undef`ed and redefined to something else?
> I am not sure what you exactly suggest here. It should not matter what COND_OP_THIRD_MACRO is defined to as we lexically compare tokens as they are written in the original source code.
> 
> Could you be a bit more specific what test case you would like to add? 
*should* not matter is my point, please show that :)


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

https://reviews.llvm.org/D55125