[PATCH] D90023: [Syntax] Add iterators over children of syntax trees.

[Eduardo Caldas via Phabricator via cfe-commits]

eduucaldas added a comment.

Thanks Sam! I learned a lot from your patch ^^



================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:157-184
+  /// Iterator over children (common base for const/non-const).
+  /// Not invalidated by tree mutations (holds a stable node pointer).
+  template <typename DerivedT, typename NodeT>
+  class child_iterator_base
+      : public llvm::iterator_facade_base<DerivedT, std::forward_iterator_tag,
+                                          NodeT> {
+  protected:
----------------
I think we should only have an iterator through `Node`s, not a general one like this.

In general `Tree` has *heterogeneous* children:
For instance, `1+2` yields the syntax tree:
```
BinaryOperatorExpression
|-  IntegerLiteralExpression
|-  Leaf
`- IntegerLiteralExpression
```

Very rarely will syntax trees have all their children of the same kind, so I don't think it makes sense to try an provide an iterator template. That makes sense for lists , because generally they *have* the same kind. But in that case we provide special iterators for lists only.



================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:202-219
+  /// child_iterator is not invalidated by mutations.
+  struct child_iterator : child_iterator_base<child_iterator, Node> {
+    using Base::Base;
+  };
+  struct const_child_iterator
+      : child_iterator_base<const_child_iterator, const Node> {
+    using Base::Base;
----------------
TL;DR:
`child_iterator` -> `ChildIterator`
`const_child_iterator` -> `ConstChildIterator`
`children` -> `getChildren`

I see you followed the naming convention of the ClangAST, which makes loads of sense. 

In syntax trees we follow the naming conventions of the [conding standard](https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly) -- just because we want consistency and we had to pick a guideline to follow -- according to that, functions should be verb like and names should be camel case.

If like us you chose `const_child_iterator` and `children` kind of "just because", could you change it to follow `ConstChildIterator` and `getChildren` respectively. 


================
Comment at: clang/lib/Tooling/Syntax/Tree.cpp:22-23
   if (auto *T = dyn_cast<syntax::Tree>(N)) {
-    for (const auto *C = T->getFirstChild(); C; C = C->getNextSibling())
-      traverse(C, Visit);
+    for (const syntax::Node &C : T->children())
+      traverse(&C, Visit);
   }
----------------
Hmm... 
That looks funny, if the user uses a range-based for loop and forgets the `&`, then we would be copying the Node???

Also in many places we had to get the address to the node. That is not really consistent with how the syntax trees API's were designed, they generally take pointers instead of references. (that said we could obviously reconsider those design choices)


================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:129
 
+TEST_F(TreeTest, Iterators) {
+  buildTree("", allTestClangConfigs().front());
----------------
Suggestion:
I would split this into `Iterators` and `ConstIterators`.


================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:139
+                                  NodeKind::TranslationUnit);
+  const syntax::Tree *ConstTree = Tree;
+
----------------
nit, feel free to ignore.


================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:159-169
+  for (unsigned I = 0; I < 3; ++I) {
+    EXPECT_EQ(It, CIt);
+    EXPECT_TRUE(It);
+    EXPECT_TRUE(CIt);
+    EXPECT_EQ(It.asPointer(), Children[I]);
+    EXPECT_EQ(CIt.asPointer(), Children[I]);
+    EXPECT_EQ(&*It, Children[I]);
----------------
Is there a reason why you didn't use a range-based for loop over Children?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90023