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