[PATCH] D67654: [clang-tidy] Fix a potential infinite loop in readability-isolate-declaration check.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

> Do we have unit tests for clang-tidy? If yes, could we also add unit-tests for this function?
>  The modified function only needs a source manager and lang options, these are easy to mock.

Unfortunately, we don't have any unit tests for all these utility functions. The modified function is only used in the `readability-isolate-declaration` check.

I agree that we should add one, will file a bug.



================
Comment at: clang-tools-extra/clang-tidy/utils/LexerUtils.h:38
 template <typename TokenKind, typename... TokenKinds>
 SourceLocation findPreviousAnyTokenKind(SourceLocation Start,
                                         const SourceManager &SM,
----------------
ilya-biryukov wrote:
> Would `findPrevious` have the same problem on the first token of the file?
> Can be hard to check without unit-tests, though.
it depends on the caller. Calling this function directly would probably get into infinite loop. As this function is only used in `readability-isolate-declaration` check, it seems that the check adds some additional guard code before calling this function, it is probably safe, I assume. (I could not figure out a case that causes the problem).


================
Comment at: clang-tools-extra/clang-tidy/utils/LexerUtils.h:66
     Optional<Token> CurrentToken = Lexer::findNextToken(Start, SM, LangOpts);
-
-    if (!CurrentToken)
+    if (!CurrentToken || CurrentToken->is(tok::eof))
       return SourceLocation();
----------------
ilya-biryukov wrote:
> We seem to be missing a corner case here: what if folks are searching for 'tok::eof'?
> 
> Currently we would return invalid location, but given the function's signature I'd expect it to return location of 'tok::eof' in that case.
> 
> I bet we don't run into this in practice, though.
yeah, I thought that we shouldn't run into this corner case, I tweaked the code to fix this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67654