[PATCH] D72581: [Syntax] Add mapping from spelled to expanded tokens for TokenBuffer

[Marcel Hlopko via Phabricator via cfe-commits]

hlopko marked 5 inline comments as done.
hlopko added a comment.

I'm submitting this patch at https://reviews.llvm.org/D77209 (with Ilya's permission). Let's continue the review there.



================
Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:228
+  /// multiple times and this function will return multiple results in those
+  /// cases. This happens when \p Spelled is inside a macro argument.
+  ///
----------------
sammccall wrote:
> Nit: move the FIXME up here? Documenting this justifies the signature, but currently it *never* happens.
Done.


================
Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:248
+  llvm::SmallVector<llvm::ArrayRef<syntax::Token>, 1>
+  expandedForSpelled(llvm::ArrayRef<syntax::Token> Spelled) const;
+
----------------
sammccall wrote:
> out of curiosity, do you have a motivating use for this function?
> 
> I think it's clear enough that it will be useful, completeness dictates it should be here, and use cases aren't always the best way to think about designing these libraries. But it'd help me understand some of the details better.
Will ask Ilya offline.


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:198
+TokenBuffer::expandedForSpelled(llvm::ArrayRef<syntax::Token> Spelled) const {
+  assert(!Spelled.empty());
+  assert(Spelled.front().location().isFileID());
----------------
sammccall wrote:
> This is a little surprising (vs returning `{}`). 
> 
> It seems plausible that you'll map file offsets -> spelled tokens -> expanded tokens, and that the middle step might "accidentally" be empty. Caller can always check, but why require them to here?
Done.


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:205
+
+  auto &File = It->second;
+  assert(File.SpelledTokens.data() <= Spelled.data());
----------------
sammccall wrote:
> nit: mind spelling this type out? it's important, not particularly verbose, and long-lived
Done.


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:209
+
+  auto *FrontMapping = mappingStartingBeforeSpelled(File, &Spelled.front());
+  unsigned SpelledFrontI = &Spelled.front() - File.SpelledTokens.data();
----------------
sammccall wrote:
> This section could use some comments about how the index calculations relate to high-level concepts.
> 
> AIUI the branches are: spelled token precedes all PP usage, spelled token is transformed by PP (subcases: macro args and other PP usage), spelled token follows PP usage.
> 
> The next section probably only needs to comment about the purpose of the divergence (+1/End to refer to one-past the region of interest).
Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72581