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