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

[Daniel Krupp via Phabricator via cfe-commits]

dkrupp marked 13 inline comments as done.
dkrupp added a comment.

Thanks for your comments. I fixed them all. I also added the handling of redundant sizeof() and alignof() operators on the way. Please check if OK now...



================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:602
+static bool compareToks(Token &T1, Token &T2, const SourceManager &SM) {
+  if (T1.getLength() != T2.getLength())
+    return false;
----------------
Szelethus wrote:
> alexfh wrote:
> > `Token::getLength()` will assert-fail for annotation tokens.
> I guess if `T1.isNot(tok::raw_identifier)` (or `T2`) we could get away with simply comparing token kinds. If they are, it wouldn't assert. :)
I changed to code as per the suggestion from Szelethus.
So we will compare only raw_identifiers char-by-char, the rest by kind only. 
According to my tests, macros and types, typedefs for example are such raw_identifiers. 



================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:605
+  return strncmp(SM.getCharacterData(T1.getLocation()),
+                 SM.getCharacterData(T2.getLocation()), T1.getLength()) == 0;
+}
----------------
alexfh wrote:
> JonasToth wrote:
> > 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.
> This code is only executed when they have the same length.
exactly, overflow should not happen.


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:638
+  SourceLocation LLoc, RLoc;
+  int LRem, RRem;
+  do {//we compare the expressions token-by-token
----------------
Szelethus wrote:
> It seems like we're only checking whether these values are `0` or not, maybe make them `bool`? Also, I find `Rem` a little cryptic, what is it referring to? Maybe `RemainingCharCount`?
> 
> Maybe we should just make a simple function, so we could both boost readability, and get rid of some these variables (`LLoc`, `RLoc`) entirely:
> 
> ```
> unsinged getCharCountUntilEndOfExpr(SourceRange ExprSR, Token T,
>                                     const SourceManager &SM) {
> 
>   assert((ExprSR.getBegin() > T.getLocation() ||
>           ExprSR.getEnd() < T.getLocation()) &&
>          "Token is not within the expression range!");
> 
>   return SM.getCharacterData(SM.getExpansionLoc(ExprSR.getEnd())) -
>          SM.getCharacterData(T.getLocation());
> }
> ```
> 
> 
good point. Refactored ass suggested.


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

https://reviews.llvm.org/D55125