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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
