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

[Haojian Wu via Phabricator via cfe-commits]

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 "syntax" in "syntax token manager" is particularly disambiguating here, both TokenBuffer and TokenManager are part of syntax, so it's not clear what it refers to (and it doens't have any obvious plain-english meaning).
> 
> Maybe some combination like `TokenBufferTokenManager` or `BufferTokenManager`.
> In fact I think best is `TokenBuffer::TokenManager`, still defined in a separate header, though I'm not sure if you think that's too weird.
Renamed to TokenBufferTokenManager. 

`BufferTokenManager` name is short, but it has `BufferToken` prefix, which seems confusing with `TokenBuffer`. `TokenBuffer::TokenManager` is weird to me, and doesn't reflect the layering IMO


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:370
+  TreeBuilder(syntax::Arena &Arena)
+      : Arena(Arena), STM(cast<SyntaxTokenManager>(Arena.getTokenManager())),
+        Pending(Arena, STM.tokenBuffer()) {
----------------
sammccall wrote:
> need changes to the public API to make this cast valid
Are you suggesting to change all public APIs where there is such a cast usage?

If yes, this seems not a trivial change, I think at least we will change all APIs (`buildSyntaxTree`, `createLeaf`, `createTree`, `deepCopyExpandingMacros`) in `BuildTree.h` (by adding a new `TokenBufferTokenManager` parameter). 
And the `Arena` probably doesn't need to have a `TokenManager` field (it could be simplified as a single `BumpPtrAllocator`), as the TokenManager is passed in parallel with the Arena.

I'm happy to do the change, but IMO, the current version doesn't seem too bad for me (if we pass an non-SyntaxTokenManager, it will trigger an assertion in debug mode).


================
Comment at: clang/lib/Tooling/Syntax/ComputeReplacements.cpp:94
                             const syntax::TranslationUnit &TU) {
-  const auto &Buffer = A.getTokenBuffer();
-  const auto &SM = A.getSourceManager();
+  const auto &STM = llvm::cast<SyntaxTokenManager>(A.getTokenManager());
+  const auto &Buffer = STM.tokenBuffer();
----------------
sammccall wrote:
> Need a change to the public interface to guarantee this cast will succeed.
> The cleanest would be just to take the SyntaxTokenManager as a param (moving the cast to the call site - I don't think Arena is otherwise used for anything).
> 
> Failing that we at least need to update the contract
Done, this is a trivial change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128411