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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

LG from my side.



================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:189
   /// EXPECTS: Child->Role != Detached
   void prependChildLowLevel(Node *Child);
   friend class TreeBuilder;
----------------
eduucaldas wrote:
> 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.
I'm not sure this is the right set of operations, but as long as it's private it's probably not worth worrying too much about.
(By the same token, I think unused functions should be dropped, but up to you).

Let's revisit if we add a public mutation API.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90240