[PATCH] D88106: [SyntaxTree] Provide iterator-like functions for Lists

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:211
+      return element == Other.element && delimiter == Other.delimiter;
+    }
   };
----------------
Please also define `operator!=` even if it is not used yet.


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:225
+  /// elements and delimiters are represented as null pointers. Below we give
+  /// examples of how this iteration would look like:
   ///
----------------
"how something looks" or "what something looks like"


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:238-273
+  class ElementAndDelimiterIterator
+      : public llvm::iterator_facade_base<ElementAndDelimiterIterator,
+                                          std::forward_iterator_tag,
+                                          ElementAndDelimiter<Node>> {
+  public:
+    ElementAndDelimiterIterator(llvm::Optional<ElementAndDelimiter<Node>> ED)
+        : EDI(ED) {}
----------------
eduucaldas wrote:
> Should we hide some of this? Most of the member functions are a couple of lines, so I decided not to. What is your opinion?
I think it is fine as is.


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:269
+      return EDI == Other.EDI;
+    }
+
----------------
Please also define `operator!=`.


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:283
+    return ElementsAndDelimitersRange(getBegin(), getEnd());
+  }
+
----------------
I'd imagine derived classes would have iterators that produce strongly-typed elements, right?

If so, these methods getBegin()/getEnd()/getRange() should have the word "Node" in them.


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:314
+private:
+  // Auxiliary methods for implementing `ElementAndDelimiterIterator`.
+  static ElementAndDelimiter<Node> getWithDelimiter(Node *Element);
----------------
People can use "find usages" to find what uses these methods. Such comments often go out of date.


================
Comment at: clang/lib/Tooling/Syntax/Tree.cpp:350
+  auto *Next = Element->getNextSibling();
+  // a  b, x => End of list, this was the last ElementAndDelimiter.
+  if (!Next)
----------------
I have a hard time understand what a b x and the comma stand for in these comments.


================
Comment at: clang/lib/Tooling/Syntax/Tree.cpp:391-396
+  auto Children = std::vector<syntax::List::ElementAndDelimiter<Node>>();
+  for (auto C : getRange()) {
+    Children.push_back(C);
   }
+
+  return Children;
----------------



================
Comment at: clang/lib/Tooling/Syntax/Tree.cpp:405
 
   return Children;
 }
----------------
Ditto?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88106