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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
