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

[Eduardo Caldas via Phabricator via cfe-commits]

eduucaldas abandoned this revision.
eduucaldas added inline comments.


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:252
+          Kind = IteratorKind::NotSentinel;
+          if (isElement(Begin))
+            Current = getWithDelimiter(cast<ElementType>(Begin));
----------------
Since `NodeRole` is forward declared, we cannot access `NodeRole::ListElement` within the header, this is my work-around.

Another possibility would be to not forward declare and put `NodeRole` definition here. I think this is a bad choice because most roles are linked to syntax.

I declare `isElement` as a static private member function of `List` from lack of better place, I accept suggestions.


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:390
+
+  ElementAndDelimiterIterator<Node> getBeforeBeginNode();
+  ElementAndDelimiterIterator<Node> getBeginNode();
----------------
I have a hard time finding a name that is expressive enough and concise enough. The current one seems to imply that we're getting a node...

Alternatives:
* `getBeforeBeginWithElementAsNode`
* `getBeforeBeginWithNodes`
* `getBeforeBeginBase`
* `getBeforeBeginNodeAndDelimiter`



================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:241-242
   /// "a; b; c"  <=> [("a" , ";"), ("b" , ";" ), ("c" , null)]
+  template <typename ElementType>
+  class ElementAndDelimiterIterator
+      : public llvm::iterator_facade_base<
----------------
eduucaldas wrote:
> Since we're gonna provide strongly-typed iterators, I make the Iterator a class template.
> 
> I keep the base functions as they were, `getElementAndDelimiterAfter...`, but I cast their result using `castToElementType`.
> 
> Another option would be to make the base functions also templated and not perform any casting.
> 
> We'll use `castToElementType` to provide the strongly-typed iterators as well.
Ignore comment above


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88106