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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:370
+  TreeBuilder(syntax::Arena &Arena)
+      : Arena(Arena), STM(cast<SyntaxTokenManager>(Arena.getTokenManager())),
+        Pending(Arena, STM.tokenBuffer()) {
----------------
hokein wrote:
> 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).
> Are you suggesting to change all public APIs where there is such a cast usage?

Yes, at the very least they should document the requirement ("this arena must use a TokenBuffer").
An unchecked downcast with no indication on the public API that a specific subclass is required just looks like a bug.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128411