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