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

hokein updated this revision to Diff 443922. hokein added a comment. remove all TokenBufferTokenManager cast usage, make it as a contract in the APIs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128411/new/ https://reviews.llvm.org/D128411 File

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

sammccall added inline comments. Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:370 + TreeBuilder(syntax::Arena &Arena) + : Arena(Arena), STM(cast(Arena.getTokenManager())), +Pending(Arena, STM.tokenBuffer()) { hokein wrote: > sammccall wrote: >

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

hokein added inline comments. Comment at: clang/include/clang/Tooling/Syntax/SyntaxTokenManager.h:20 +/// It tracks the underlying token buffers, source manager, etc. +class SyntaxTokenManager : public TokenManager { +public: sammccall wrote: > I don't think "syn

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

hokein updated this revision to Diff 443737. hokein added a comment. more update Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128411/new/ https://reviews.llvm.org/D128411 Files: clang/include/clang/Tooling/Syntax/Mutations.h clang/include/cla

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

hokein updated this revision to Diff 443736. hokein marked 7 inline comments as done. hokein added a comment. address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128411/new/ https://reviews.llvm.org/D128411 Files: clang/include/clang

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

sammccall added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:24 #include "clang/Basic/TokenKinds.h" #include "clang/Lex/Token.h" I think Token and TokenKinds are also no longer needed? Comment at: clang/include/cl

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

hokein added inline comments. Comment at: clang/include/clang/Tooling/Syntax/TokenManager.h:37 + + // FIXME: add an interface for getting token kind. +}; sammccall wrote: > I wouldn't want to prejudge this: this is a very basic attribute similar to > kind/role,

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

hokein updated this revision to Diff 443173. hokein marked 9 inline comments as done. hokein added a comment. Herald added a subscriber: mgorny. address review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128411/new/ https://reviews.llvm

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

ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:463 +/// It tracks the underlying token buffers, source manager, etc. +class SyntaxTokenManager : public TokenManager { +public: sammccall wrote: > conceptually this is j

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

sammccall added inline comments. Comment at: clang/include/clang/Tooling/Syntax/TokenManager.h:9 +// +// Defines Token interfaces for the syntax-tree, decoupling the syntax-tree from +// the TokenBuffer. It enables producers (e.g. clang pseudoparser) to produce a ---

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

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)

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

hokein added inline comments. Comment at: clang/include/clang/Tooling/Syntax/TokenManager.h:33 + /// The syntax-tree Leaf node holds a Key. + using Key = const void *; + /// Gets the text of token identified by the key. ilya-biryukov wrote: > I have just reali

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

ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Syntax/TokenManager.h:33 + /// The syntax-tree Leaf node holds a Key. + using Key = const void *; + /// Gets the text of token identified by the key. I have just realized that we were

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

ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Syntax/TokenManager.h:23 + +/// Base token interfaces for the syntax-tree. +class TokenManager { NIT: maybe explain the `TokenManager` concept here, the comment seems to be a leftover f

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

hokein added a comment. I'm quite happy about the interfaces now, it should be in a good shape for the API review (would be nice to get some initial feedback before I do further cleanup). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128411/new/

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

hokein updated this revision to Diff 439705. hokein added a comment. Revised to the TokenManager approach: - Inroduce a Base Token class (TokenManager) for syntax-tree, the motivation is to allow using different underlying token implementation in syntax-tree - Decouple the syntax-tree from the T

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

sammccall added a comment. As discussed offline this has some problems: - putting virtual methods on BaseToken gives it a vtable, which makes it (and syntax::Token) large - being able to use ArrayRef but not ArrayRef is a bit weird - unusual uses of inheritance can be hard to reason about We s

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

hokein created this revision. hokein added reviewers: sammccall, ilya-biryukov. Herald added a project: All. hokein requested review of this revision. Herald added a project: clang. This enables us to use a different underlying token implementation for the syntax Leaf node -- in clang pseudoparser