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

[Jonas Toth via Phabricator via cfe-commits]

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