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