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