[PATCH] D99086: [clang][Syntax] Optimize expandedTokens for token ranges.

[Sam McCall via Phabricator via cfe-commits]

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