[PATCH] D90240: [SyntaxTree] Add reverse links to syntax Nodes.

[Eduardo Caldas via Phabricator via cfe-commits]

eduucaldas marked 2 inline comments as done.
eduucaldas added inline comments.


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:109
   Node *getNextSibling() { return NextSibling; }
+  const Node *getPreviousSibling() const { return PreviousSibling; }
+  Node *getPreviousSibling() { return PreviousSibling; }
----------------
sammccall wrote:
> (Not something to change in this patch...)
> Since adding the "get" prefixes, getNextSibling() and now getPreviousSibling() are pretty verbose, we might consider getNext()/getPrevious()
I agree




================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:189
   /// EXPECTS: Child->Role != Detached
   void prependChildLowLevel(Node *Child);
   friend class TreeBuilder;
----------------
sammccall wrote:
> gribozavr2 wrote:
> > eduucaldas wrote:
> > > Should we provide an `appendChildLowLevel` as well?
> > > 
> > > That has one use inside `foldChildren` in `BuildTree.cpp`. 
> > > Currently this function does a reverse iteration prepending children. We could change that into a forward iteration appending. There is no impact in time-complexity. This change would just improve readability inside this function.
> > There is some awkwardness in foldChildren because we can only go in reverse -- maybe append is indeed more natural.
> Consider `insert(Node *Child, const Node *Before)` where Before=Null means append.
> 
> This is fairly ergonomic for common cases: 
>  - append: `insert(N, null)`
>  - prepend: `insert(N, N->firstChild())`
>  - insert-before: `insert(N, M)`
>  - insert-after: `insert(N, M->nextSibling())`
> 
> (Either before or after works fine, before matches STL insert better)
That is great, but we have even a superset of this:
`replaceChildRangeLowLevel(Node* BeforeBegin, Node* End, Node* New)`
where:
`insert(Child, Before) = replaceChildRangeLowLevel(Before, Before->getNextSibling(), Child)`

I think the point of having append and prepend is that until now that's what builders need, and such a specialization carries more semantics.

For the mutations API, where we need this kind of capability we provide `replaceChildRangeLowLevel`.


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:189
   /// EXPECTS: Child->Role != Detached
   void prependChildLowLevel(Node *Child);
   friend class TreeBuilder;
----------------
eduucaldas wrote:
> sammccall wrote:
> > gribozavr2 wrote:
> > > eduucaldas wrote:
> > > > Should we provide an `appendChildLowLevel` as well?
> > > > 
> > > > That has one use inside `foldChildren` in `BuildTree.cpp`. 
> > > > Currently this function does a reverse iteration prepending children. We could change that into a forward iteration appending. There is no impact in time-complexity. This change would just improve readability inside this function.
> > > There is some awkwardness in foldChildren because we can only go in reverse -- maybe append is indeed more natural.
> > Consider `insert(Node *Child, const Node *Before)` where Before=Null means append.
> > 
> > This is fairly ergonomic for common cases: 
> >  - append: `insert(N, null)`
> >  - prepend: `insert(N, N->firstChild())`
> >  - insert-before: `insert(N, M)`
> >  - insert-after: `insert(N, M->nextSibling())`
> > 
> > (Either before or after works fine, before matches STL insert better)
> That is great, but we have even a superset of this:
> `replaceChildRangeLowLevel(Node* BeforeBegin, Node* End, Node* New)`
> where:
> `insert(Child, Before) = replaceChildRangeLowLevel(Before, Before->getNextSibling(), Child)`
> 
> I think the point of having append and prepend is that until now that's what builders need, and such a specialization carries more semantics.
> 
> For the mutations API, where we need this kind of capability we provide `replaceChildRangeLowLevel`.
I replace every place where we did a reverse iteration prepending for a normal iteration appending, and now there are no more users of prepend ^^. 

I propose we keep it anyways, we have bidirection list, makes sense to have both.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90240