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

Daniel Krupp via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Dec 21 06:13:02 PST 2018

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.



More information about the cfe-commits mailing list