[PATCH] D128411: [syntax] Introduce a BaseToken class.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:569
   struct Forest {
-    Forest(syntax::Arena &A) {
-      assert(!A.getTokenBuffer().expandedTokens().empty());
-      assert(A.getTokenBuffer().expandedTokens().back().kind() == tok::eof);
+    Forest(syntax::Arena &A, const syntax::SyntaxTokenManager &STM) {
+      assert(!STM.getTokenBuffer().expandedTokens().empty());
----------------
Could we accept a TokenBuffer here directly?
If TokenManager is needed, we can use it directly in the arena.


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:625
     /// Add \p Node to the forest and attach child nodes based on \p Tokens.
-    void foldChildren(const syntax::Arena &A, ArrayRef<syntax::Token> Tokens,
+    void foldChildren(const syntax::SyntaxTokenManager &STM, ArrayRef<syntax::Token> Tokens,
                       syntax::Tree *Node) {
----------------
Same suggestion here: accept TokenBuffer instead of TokenManager


================
Comment at: clang/lib/Tooling/Syntax/Tree.cpp:271
+      // FIXME: can be fixed by adding an tok::Kind in the Leaf node.
+      // assert(cast<Leaf>(C).getToken()->kind() == L->getDelimiterTokenKind());
     }
----------------
hokein wrote:
> ilya-biryukov wrote:
> > Maybe add `TokenManager::getKind(Key)` right away and remove this FIXME.
> > This should as simple as `cast<syntax::Token>(T)->Kind`, right? Or am I missing some complications?
> Yeah, the main problem is that we don't have the `TokenManager` object in the `syntax::Node`, we somehow need to pass it (e.g. a function parameter), which seems a heavy change. I'm not sure it is worth for this small assert.
That makes sense. WDYT about the alternative fix: to pass ̀TokenManager` to `assertInvariants`?
Not necessary to do it now, just thinking about changing the FIXME


================
Comment at: clang/unittests/Tooling/Syntax/TreeTestBase.cpp:116
     syntax::TranslationUnit *&Root;
+    std::unique_ptr<syntax::SyntaxTokenManager> &TM;
     std::unique_ptr<syntax::TokenBuffer> &TB;
----------------
NIT: it´s not breaking anything now, but I suggest putting SyntaxTokenManager after TokenBuffer.
The reason is that it´s the right destruction order, TokenManager has references to TokenBuffer, so it could potentially access it in destructor some time in the future (e.g. imagine asserting something on tokens).

Not that it actually breaks today, but seems like a potential surprising bug in the future if we happen to refactor code in a certain way. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128411