sammccall added inline comments.
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:25
/// A kind of a syntax node, used for implementing casts.
enum class NodeKind : uint16_t {
Leaf,
----------------
there are going to be many of these. I'd suggest either sorting them all, or
breaking them into blocks (e.g. statements vs declarations vs leaf/tu/etc) and
sorting those blocks.
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:99
/// An abstract node for C++ statements, e.g. 'while', 'if', etc.
class Statement : public Tree {
public:
----------------
Do you want to expose the statement-ending-semicolon here?
(Not all statements have it, but common enough you may want it in the base
class instead of all children)
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:144
+ syntax::Leaf *switchKeyword();
+ syntax::Statement *body();
+
----------------
The fact that this can be an arbitrary statement is kind of shocking. But
apparently true!
In the long run, we're probably going to be able to find the case statements
somehow, even though they're not part of the immediate grammar. Not sure
whether this should be via the regular AST or by adding links here. Anyway,
problem for another day.
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:159
+ }
+ syntax::Leaf *caseKeyword();
+ syntax::Statement *body();
----------------
expression for the value?
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:160
+ syntax::Leaf *caseKeyword();
+ syntax::Statement *body();
+
----------------
syntactically, is it useful to model the body as a single statement? It's not a
CompoundStmt as it has no braces. Seems like a sequence...
Or is the idea that the first following statement is the body (might be
nothing), and subsequent ones aren't part of the body? Why is this more useful
than making the body a sibling?
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:176
+ syntax::Leaf *defaultKeyword();
+ syntax::Statement *body();
+
----------------
might be handy to unify this with CaseStatement somehow (a base class, or make
it literally a CaseStatement with a null body and a bool isDefaultCase() that
looks at the keyword token)
Mostly thinking about code that's going to iterate over case statements.
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:185
+/// if (cond) <then-statement> else <else-statement>
+class IfStatement final : public Statement {
+public:
----------------
I guess the missing cond here (and similar below) are due to complexities
around the variable declaring variants?
Warrants a FIXME I think
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:193
+ syntax::Statement *thenStatement();
+ syntax::Leaf *elseKeyword();
+ syntax::Statement *elseStatement();
----------------
I think throughout it's important to mark which of these are:
- nullable in correct code
- nullable in code generated by recovery
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:265
+/// return <expr>;
+class ReturnStatement final : public Statement {
+public:
----------------
(any reason we can't already have the expr here?)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63835/new/
https://reviews.llvm.org/D63835
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
