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