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

[Sam McCall via Phabricator via cfe-commits]

sammccall 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; }
----------------
(Not something to change in this patch...)
Since adding the "get" prefixes, getNextSibling() and now getPreviousSibling() are pretty verbose, we might consider getNext()/getPrevious()


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:189
   /// EXPECTS: Child->Role != Detached
   void prependChildLowLevel(Node *Child);
   friend class TreeBuilder;
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90240