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

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Jun 27 02:40:34 PDT 2022

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 {
sammccall wrote:
> conceptually this is just "TokenBuffer implements TokenManager"
> The main reason I can see not to actually write that is to avoid the dependency from TokenBuffer (tokens library) to TokenManager (syntax library). But here you've added that dependency anyway.
> So I think we'd be better either with `TokenBuffer : TokenManager` or moving this class to its own header.
I would argue they should be separate concepts.
- `TokenBuffer` is about storing tokens and mapping between expanded and spelled token streams.
- `SyntaxTokenManager` is about implementing relevant certain operations on `syntax::Token` and hiding the actual token type.
Conceptually those things are different and decoupling them allows for more flexibility and allows reasoning about them independently. In particular, one could imagine having a `SyntaxTokenManager` without a `TokenBuffer` if we do not need to deal with two streams of tokens.

I would suggest keeping `SyntaxTokenManager` as a separate class.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list