JonasToth added inline comments.
================ Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:605 + return strncmp(SM.getCharacterData(T1.getLocation()), + SM.getCharacterData(T2.getLocation()), T1.getLength()) == 0; +} ---------------- This operation could overflow if `len(T1) > len(T2)` and `T2` is the last token of the file. I think `std::min(T1.getLength(), T2.getLength())` would be better. ================ Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:608 + +//Returns true if both LhsEpxr and RhsExpr are +//macro expressions and they are expanded ---------------- Nit: Please add whitespace after // and use /// for the function documentation. ================ Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:629 + + const char *LTokenPos = MB->getBufferStart() + LsrLocInfo.second; + const char *RTokenPos = MB->getBufferStart() + RsrLocInfo.second; ---------------- Please format with clang-format, some places seem to be a little off whitespace-wise. ================ 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: > > 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 :) > undef/define testcase added and passes. @JonasToth is this what you thought > of? yup. thank you 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