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