sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land.
Just doc nits - I think maybe there's some confusion on what a token range is. Code looks good though! ================ Comment at: clang-tools-extra/clangd/ParsedAST.cpp:426 syntax::TokenBuffer Tokens = std::move(CollectTokens).consume(); + // Index expanded tokens to optimize finding expandedToken in token ranges. + // Primarily useful for building a SelectionTree. ---------------- nit: the first sentence is just repeating the function's doc - I'd just say "Makes SelectionTree build much faster" ================ Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:196 + /// Creates an index ExpandedTokens by their SourceLocation for faster + /// lookups. This optimizes future calls to `expandedTokens(SourceRange)` for ---------------- Hmm, the primary doc (first sentence) should probably be about the effect rather than implementation details. Maybe just "Builds a cache to make expandedToken(SourceRange) faster."? ================ Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:203 /// Returns the subrange of expandedTokens() corresponding to the closed - /// token range R. + /// source range R. + /// Consider calling indexExpandedTokens() before for faster lookups for ---------------- token range was correct and critical to mention here, please revert It means that R.endLocation() is the last token included in the range. Sometimes endLocation() means other things, in closed character range, or an half-open token or character range. ================ Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:205 + /// Consider calling indexExpandedTokens() before for faster lookups for + /// 'token ranges'. llvm::ArrayRef<syntax::Token> expandedTokens(SourceRange R) const; ---------------- can you drop "for 'token ranges'" here? I'm not sure what it means, and all ranges passed in here must be token ranges per contract. ================ Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:379 + // Index of ExpandedTokens for faster lookups by SourceLocation. This is + // useful while finding expanded tokens in a 'token range'. + llvm::DenseMap<SourceLocation, unsigned> ExpandedTokIndex; ---------------- again this comment is confusing. I'd suggest just dropping it - it's fairly covered by indexExpandedTokens()'s comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99086/new/ https://reviews.llvm.org/D99086 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
