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