dkrupp marked 3 inline comments as done. dkrupp added a comment. In D55125#1314788 <https://reviews.llvm.org/D55125#1314788>, @JonasToth wrote:
> In D55125#1314741 <https://reviews.llvm.org/D55125#1314741>, @Szelethus wrote: > > > @JonasToth this is the `Lexer` based expression equality check I talked > > about in D54757#1311516 <https://reviews.llvm.org/D54757#1311516>. The > > point of this patch is that the definition is a macro sure could be build > > dependent, but the macro name is not, so it wouldn't warn on the case I > > showed. What do you think? > > > Yes, this approach is possible. > IMHO it is still a bug/redudant if you do the same thing on both paths and > warning on it makes the programmer aware of the fact. E.g. the macros might > have been something different before, but a refactoring made them equal and > resulted in this situation. @JonasThoth: I see you point with refactoring, but let's imagine that the two macros COND_OP_MACRO and COND_OP_THIRD_MACRO are defined by compile time parameters. If the two macros are happened to be defined to the same value the user would get a warning, and if not she would not. How would the user would "fix" her code in the first case? So if the macro names are different in the conditional expression, it is not a redundant expression because the macro names are compile time parameters (with just eventually same values). This was the same logic the old test case were testing: k += (y < 0) ? COND_OP_MACRO : COND_OP_OTHER_MACRO; ================ 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) { ---------------- 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? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55125/new/ https://reviews.llvm.org/D55125 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
