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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:188
+  /// Similar but appends.
+  void appendChildLowLevel(Node *Child, NodeRole Role);
+
----------------
This is a complete nitpick, but could you put append before prepend? I think a lot more people are interested in append. Same in the two overloads below.


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:201
   /// (!) \p New can be null to model removal of the child range.
-  void replaceChildRangeLowLevel(Node *BeforeBegin, Node *End, Node *New);
+  void replaceChildRangeLowLevel(Node *Begin, Node *End, Node *New);
   friend class MutationsImpl;
----------------
It would be great to document that nullptr in Begin or End mean the one-past-end position.


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:109
   Node *getNextSibling() { return NextSibling; }
+  const Node *getPreviousSibling() const { return PreviousSibling; }
+  Node *getPreviousSibling() { return PreviousSibling; }
----------------
eduucaldas wrote:
> 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
> 
> 
I like having the word "sibling" in the name -- getNext() makes me wonder, "next what?"

If this was an iterator abstraction where next/previous are core to the meaning, sure, but a tree node is more complex.


================
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.
> That is great, but we have even a superset of this:

I mean, if we intend to provide a convenient API, adding `insert` would make sense even if it is not needed right now. From the industry experience with containers, we know that replace, insert, append, prepend are all common operations, and even though all of them can be expressed with just replace, people like to use specialized operations as that expresses the intent better.

OTOH, it seems like the design here is that mutations are provided by the MutationsImpl class, so any such convenience operation needs to be implemented there.


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:639
 
       // We need to go in reverse order, because we can only prepend.
+      for (auto It = BeginChildren; It != EndChildren; ++It) {
----------------
Remove the comment?


================
Comment at: clang/lib/Tooling/Syntax/Synthesis.cpp:204
+  for (const auto *ChildIt = Children.begin(); ChildIt != Children.end();
+       ++ChildIt)
+    FactoryImpl::appendChildLowLevel(T, ChildIt->first, ChildIt->second);
----------------
Use a range-based for loop?


================
Comment at: clang/lib/Tooling/Syntax/Tree.cpp:128
                                              Node *New) {
-  assert((!BeforeBegin || BeforeBegin->Parent == this) &&
-         "`BeforeBegin` is not a child of `this`.");
+  assert(((!Begin && !FirstChild) || Begin->Parent == this) &&
+         "`Begin` is not a child of `this`.");
----------------
Seems like Begin==nullptr is only allowed when this subtree has no children. I'd think that Begin==End==nullptr is a valid way to append children even to non-empty trees.

Do we have tests for this function? Would be really good to add that case.


================
Comment at: clang/lib/Tooling/Syntax/Tree.cpp:159
 
+  // Point to node before the range to be removed, to later insert the `New`
+  // range after it.
----------------



================
Comment at: clang/lib/Tooling/Syntax/Tree.cpp:169-170
     N->Parent = nullptr;
     N->NextSibling = nullptr;
+    N->PreviousSibling = nullptr;
     if (N->Original)
----------------
Thinking about the semantics of this operation, do we need to break the sibling links in the range that is being removed?  We expect the sibling links to be established in the new range. That's a suspicious asymmetry.



================
Comment at: clang/lib/Tooling/Syntax/Tree.cpp:178
+  // Attach new range.
+  auto *&NewBegin = BeforeBegin ? BeforeBegin->NextSibling : FirstChild;
+  auto *&NewLast = End ? End->PreviousSibling : LastChild;
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90240