ilya-biryukov added a comment.
Still have a few comments to address in `TokenCollector` and wrt to naming. But
apart from this revision is ready for another round.
================
Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:120
+ /// the original source file. The tranformation may not be possible if the
+ /// tokens cross macro invocations in the middle, e.g.
+ /// #define FOO 1*2
----------------
gribozavr wrote:
> "The mapping fails if cross boundaries of macro expansions, that is, don't
> correspond to a complete top-level macro invocation"
>
> Consider adding examples.
>
> ```
> Given this source file:
>
> #define FIRST f1 f2 f3
> #define SECOND s1 s2 3
>
> a FIRST b SECOND c // expansion: a f1 f2 f3 b s1 s2 s3 c
>
> toOffsetRange will map tokens like this:
>
> input range => output range
> a => a
> s1 s2 s3 => SECOND
> a f1 f2 f3 => a FIRST
> a f1 => can't map
> s1 s2 => s1 s2 within the macro definition
> ```
>
> Could you add these to tests as well?
Done. Note that we never map to tokens inside macro definition (the `s1 s2`
example)
================
Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:191
+ /// i.e. after running Execute().
+ TokenBuffer consume() &&;
+
----------------
gribozavr wrote:
> gribozavr wrote:
> > "Finalizes token collection"
> >
> > Of course a function called "consume" consumes the result :)
> LLVM_NODISCARD?
> Of course a function called "consume" consumes the result :)
Agreed :-)
================
Comment at: clang/lib/Tooling/Syntax/TokenBuffer.cpp:295
+MacroExpansion::tokens(const TokenBuffer &B) const {
+ return B.tokens().slice(BeginExpansionToken,
+ EndExpansionToken - BeginExpansionToken);
----------------
gribozavr wrote:
> "ExpansionTokenBegin"? "ExpansionTokenStartIndex"?
Went with `BeginExpandedToken`, `EndExpandedToken`.
================
Comment at: clang/unittests/Tooling/Syntax/TokenBufferTest.cpp:190
+ for (unsigned I = 0; I < Actual.size(); ++I) {
+ auto &A = Actual[I];
+ auto &E = Expected[I];
----------------
Eugene.Zelenko wrote:
> Return type is not obvious, so auto should not be used.
The declarations of `Actual` and `Expected` are **really** close, both types
are easy to infer
================
Comment at: clang/unittests/Tooling/Syntax/TokenBufferTest.cpp:429
+ Code = llvm::Annotations(R"cpp(
+ $all[[int $a[[a]] = $numbers[[100 + 200]];]]
+ )cpp");
----------------
gribozavr wrote:
> Could we instead use some nonsensical code like "a1 a2 a3 FIRST a3 a4 a5
> SECOND a6 a7 a8", and instead of `OfKind` we can make a helper that finds the
> identifier with the given name?
Thanks. Much nicer with a function that finds by text.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59887/new/
https://reviews.llvm.org/D59887
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
