[PATCH] D61637: [Syntax] Introduce syntax trees

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/include/clang/Tooling/Syntax/Corpus.h:23
+/// token buffers, source manager, etc.
+class Corpus {
+public:
----------------
I think plain SyntaxArena might be a better name here :-/
Corpus refers to texts (the use in dex is by analogy, as we call symbols "documents" from search).



================
Comment at: clang/include/clang/Tooling/Syntax/Corpus.h:26
+  Corpus(SourceManager &SourceMgr, const LangOptions &LangOpts,
+         TokenBuffer MainFile);
+
----------------
MainFile is presumably the whole TU, name might need a tweak.
Can it be empty?
The relationship between Corpus and TokenBuffer seems a little weird. Why is it needed?


================
Comment at: clang/include/clang/Tooling/Syntax/Corpus.h:38
+  std::pair<FileID, llvm::ArrayRef<syntax::Token>>
+  tokenizeBuffer(std::unique_ptr<llvm::MemoryBuffer> Buffer);
+
----------------
Are you planning to have a way to add tokens directly? Having to turn them into text and re-lex them seems like it might be inconvenient.


================
Comment at: clang/include/clang/Tooling/Syntax/Corpus.h:40
+
+  /// Construct a new syntax node of a specified kind. The memory for a node is
+  /// owned by the corpus and will be freed when the corpus is destroyed.
----------------
Now there's two ways to do this: `new (C.allocator()) T(...)` or `C.construct<T>(...)`. Do we need both?

(If we do, is the syntax `new (C) T(...)` more natural?)


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:34
+
+/// Build a syntax tree for the main file.
+TranslationUnit *buildSyntaxTree(Corpus &C,
----------------
for a translation unit? or for only decls within the main file?


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:40
+/// node.
+void traverse(Node *N, llvm::function_ref<void(Node *)> Visit);
+void traverse(const Node *N, llvm::function_ref<void(const Node *)> Visit);
----------------
I've been burned with adding these APIs without use cases.

It seems likely you want a way to:
 - skip traversal of children
 - abort the traversal entirely


================
Comment at: clang/include/clang/Tooling/Syntax/Tree/Cascade.h:1
+//===- Tree.h - cascade of the syntax tree --------------------*- C++ -*-=====//
+//
----------------
this is Cascade.h, not tree.h


================
Comment at: clang/include/clang/Tooling/Syntax/Tree/Cascade.h:1
+//===- Tree.h - cascade of the syntax tree --------------------*- C++ -*-=====//
+//
----------------
sammccall wrote:
> this is Cascade.h, not tree.h
why "cascade"?


================
Comment at: clang/include/clang/Tooling/Syntax/Tree/Cascade.h:1
+//===- Tree.h - cascade of the syntax tree --------------------*- C++ -*-=====//
+//
----------------
sammccall wrote:
> sammccall wrote:
> > this is Cascade.h, not tree.h
> why "cascade"?
The Tree/ subdirectory seems superfluous - why are these separate from Syntax/?


================
Comment at: clang/include/clang/Tooling/Syntax/Tree/Cascade.h:74
+/// A composite tree node that has children.
+class TreeNode : public Node {
+public:
----------------
This use of "tree node" to mean specifically internal node seems confusing - is it common?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61637