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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:24
 
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Lex/Token.h"
----------------
I think Token and TokenKinds are also no longer needed?


================
Comment at: clang/include/clang/Tooling/Syntax/SyntaxTokenManager.h:20
+/// It tracks the underlying token buffers, source manager, etc.
+class SyntaxTokenManager : public TokenManager {
+public:
----------------
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.


================
Comment at: clang/include/clang/Tooling/Syntax/TokenManager.h:14
+// implementation. It enables producers (e.g. clang pseudoparser) to produce a
+// syntax-tree with a different underlying token implementation.
+//
----------------
unclear: different from what? it's not clear what "enables" means if there's no default. Maybe replace the sentence with:

"For example, a TokenBuffer captured from a clang parse may track macro expansions and associate tokens with clang's SourceManager, while a pseudo-parser would use a flat array of raw-lexed tokens in memory."


================
Comment at: clang/include/clang/Tooling/Syntax/TokenManager.h:38
+  using Key = uintptr_t;
+  /// Gets the text of token identified by the key.
+  virtual llvm::StringRef getText(Key K) const = 0;
----------------
This is not a useful comment, either remove it or add more content to make it useful.

In particular the guarantees (or lack thereof) of exactly what this text is would be helpful. (This is some source code that would produce this token, though it may differ from exactly what was spelled in the file when preprocessing is involved)?


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:9
 // Defines the basic structure of the syntax tree. There are two kinds of nodes:
-//   - leaf nodes correspond to a token in the expanded token stream,
+//   - leaf nodes correspond to a token key in the token manager
 //   - tree nodes correspond to language grammar constructs.
----------------
this is an implementation detail.
At a high level, leaf nodes correspond to tokens. I'd just delete "expanded" from the original comment


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:46
 private:
-  SourceManager &SourceMgr;
-  const LangOptions &LangOpts;
-  const TokenBuffer &Tokens;
-  /// IDs and storage for additional tokenized files.
-  llvm::DenseMap<FileID, std::vector<Token>> ExtraTokens;
+  // Manage all token-related stuff.
+  TokenManager& TokenMgr;
----------------
this is not a helpful comment, just remove it?


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:370
+  TreeBuilder(syntax::Arena &Arena)
+      : Arena(Arena), STM(cast<SyntaxTokenManager>(Arena.getTokenManager())),
+        Pending(Arena, STM.tokenBuffer()) {
----------------
need changes to the public API to make this cast valid


================
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();
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128411