carlosgalvezp accepted this revision. carlosgalvezp added a comment. This revision is now accepted and ready to land.
> 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! ================ Comment at: clang-tools-extra/clang-tidy/utils/LexerUtils.cpp:84-85 + Lexer::findNextToken(Start, SM, LangOpts); + if (!CurrentToken || !CurrentToken->is(tok::comment)) + return CurrentToken; + ---------------- PiotrZSL wrote: > carlosgalvezp wrote: > > 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; > > ``` > Current code is correct, it's inversion of what we had in while, simply if no > token, or token is not comment, then leave. You are right, nevermind! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146881/new/ https://reviews.llvm.org/D146881 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
