[clang-tidy] Fix findNextTokenSkippingComments & rangeContainsExpansionsOrDirectives

[PiotrZSL]

Summary

findNextTokenSkippingComments is actually a endless loop,
implementing it correctly.
rangeContainsExpansionsOrDirectives were skiping every second
token, if there were no whitespaces bettwen tokens.

[carlosgalvezp]

I'm surprised we don't have unit tests that catch this?

clang-tools-extra/clang-tidy/utils/LexerUtils.cpp

84–85

I'm not sure this logic is correct - if (CurrentToken is there to ensure that CurrentToken is not a nullopt before dereferencing via CurrentToken->is. It should be:

if (CurrentToken && !CurrentToken->is(tok::comment))
  return CurrentToken;

99

This is a bit unintuitive if we compare it with e.g. STL iterators - end() is always "one-past" the end. What's the convention for SourceRange, does end contain a valid end range, or one-past-the-end?

[PiotrZSL]

clang-tools-extra/clang-tidy/utils/LexerUtils.cpp

84–85

Current code is correct, it's inversion of what we had in while, simply if no token, or token is not comment, then leave.

99

In SourceRange end is pointing to last token, not to last token + 1.
This is inclusive range.

[PiotrZSL]

We don't have unit tests for most of code... It's usually tested indirectly by checks... But this code is used I think only in 1-2 checks.

[carlosgalvezp]

It's usually tested indirectly by checks... But this code is used I think only in 1-2 checks.

I see. I'm still trying to understand why we haven't seen this before. If the function is an endless loop, shouldn't the checks that use this function end up in an endless loop too? Would be interesting to try and reproduce an endless loop in the tests of one of the checks that use this. Anyway the patch is fine as is, thanks for fixing!

clang-tools-extra/clang-tidy/utils/LexerUtils.cpp

84–85

You are right, nevermind!

clang-tools-extra/clang-tidy/utils/LexerUtils.cpp

 SourceLocation findNextTerminator(SourceLocation Start, const SourceManager &SM,
                                   const LangOptions &LangOpts) {
   return findNextAnyTokenKind(Start, SM, LangOpts, tok::comma, tok::semi);
 }
 
 std::optional<Token>
 findNextTokenSkippingComments(SourceLocation Start, const SourceManager &SM,
                               const LangOptions &LangOpts) {
-  std::optional<Token> CurrentToken;
-  do {
-    CurrentToken = Lexer::findNextToken(Start, SM, LangOpts);
-  } while (CurrentToken && CurrentToken->is(tok::comment));
+  while (Start.isValid()) {
+    std::optional<Token> CurrentToken =
+        Lexer::findNextToken(Start, SM, LangOpts);
+    if (!CurrentToken || !CurrentToken->is(tok::comment))
       return CurrentToken;
+
+    Start = CurrentToken->getLocation();
+  }
+
+  return std::nullopt;
 }
 
 bool rangeContainsExpansionsOrDirectives(SourceRange Range,
                                          const SourceManager &SM,
                                          const LangOptions &LangOpts) {
   assert(Range.isValid() && "Invalid Range for relexing provided");
   SourceLocation Loc = Range.getBegin();
 
-  while (Loc < Range.getEnd()) {
+  while (Loc <= Range.getEnd()) {
     if (Loc.isMacroID())
       return true;
 
     std::optional<Token> Tok = Lexer::findNextToken(Loc, SM, LangOpts);
 
     if (!Tok)
       return true;
 
     if (Tok->is(tok::hash))
       return true;
 
-    Loc = Lexer::getLocForEndOfToken(Loc, 0, SM, LangOpts).getLocWithOffset(1);
+    Loc = Tok->getLocation();
   }
 
   return false;
 }
 
 std::optional<Token> getQualifyingToken(tok::TokenKind TK,
                                         CharSourceRange Range,
                                         const ASTContext &Context,