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

[Ilya Biryukov via Phabricator via cfe-commits]

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 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128411