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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added a comment.

Submitting a few comments to start up the discussions.

The actual changes will follow.



================
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:
----------------
sammccall wrote:
> 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)
Yes, only "leaf" (i.e. the ones not having any statement children) have it.
I was thinking about:
  - having a separate class for non-composite statements and providing an accessor there,
  - providing an accessor in each of the leaf statements (would mean some duplication, but, arguably, better discoverability).

But, from an offline conversation, we seem to disagree that inheritance is a proper way to model this.
Would it be ok to do this in a follow-up? I'll add a FIXME for now.


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:160
+  syntax::Leaf *caseKeyword();
+  syntax::Statement *body();
+
----------------
sammccall wrote:
> 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?
This models the structure of the C++ grammar (and clang AST).
Getting from a switch statements to all its `case` and `default` labels seems useful, but should be addressed by a separate API that traverses the corresponding syntax tree nodes.

Marking as done, from an offline conversation we seem to agree here.
Feel free to reopen if needed.


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:176
+  syntax::Leaf *defaultKeyword();
+  syntax::Statement *body();
+
----------------
sammccall wrote:
> 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.
I would model with with a base class, but let's agree whether that's the right way to approach this.


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:193
+  syntax::Statement *thenStatement();
+  syntax::Leaf *elseKeyword();
+  syntax::Statement *elseStatement();
----------------
sammccall wrote:
> I think throughout it's important to mark which of these are:
>  - nullable in correct code
>  - nullable in code generated by recovery
I would suggest to only mark the nodes that are nullable in the correct code. For recovery, I would assume the following rule (please tell me if I'm wrong):

On a construct whose parsing involved recovery:
- if the node has an introducing token (`if`, `try`, etc.), the corresponding child cannot be null.
- any other child can be null.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63835