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