[PATCH] D63835: [Syntax] Add nodes for most common statements

[Sam McCall via Phabricator via cfe-commits]

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