[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

[Ilya Biryukov via Phabricator via cfe-commits]

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