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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:601
 
+static bool compareToks(Token &T1, Token &T2, const SourceManager &SM) {
+  if (T1.getLength() != T2.getLength())
----------------
alexfh wrote:
> Should this function compare token kinds first?
I personally prefer to see boolean functions to have a name that starts with either "should", "is", "does", "has", or anything that clearly indicates that it returns with either `true` or `false`. In this case, "compare" is especially misleading, since it might as well return `-1`, `0` or `1`.

Maybe `hasSameLength`?


================
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;
----------------
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. :)


================
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:638
+  SourceLocation LLoc, RLoc;
+  int LRem, RRem;
+  do {//we compare the expressions token-by-token
----------------
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());
}
```




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

https://reviews.llvm.org/D55125