[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/AST/Expr.h:5099
+  public:
+    using iterator_category = std::forward_iterator_tag;
+    using value_type = AssociationTy<Const>;
----------------
Carrying over the conversation from D57098:

>> @aaron.ballman It seems like this should be pretty trivial to make into a random access iterator, or am I missing something?
> @riccibruno Indeed, at the cost of some boilerplate. I can totally do this but from what I understood the motivation was to use this in ranges, and for this a forward iterator is good enough (although see the next comment)

You are correct, the primary motivation are range-based APIs. However, the iterator-based API should still behave in a reasonable manner and some algorithms do better with random access iterators than forward only. Even a bidirectional iterator would be preferred because I can at least use `std::prev()` on it.


================
Comment at: include/clang/AST/Expr.h:5103
+    using reference = AssociationTy<Const>;
+    using pointer = AssociationTy<Const>;
+    AssociationIteratorTy() = default;
----------------
Carrying over the conversation from D57098:

>> @aaron.ballman Cute, but I suspect this may come back to bite us at some point. For instance, if someone thinks they're working with a real pointer, they're likely to expect pointer arithmetic to work when it won't (at least they'll get compile errors though).
> @riccibruno Hmm, but pointer is just the return type of operator-> no ? Is it actually required to behave like a pointer ? The only requirement I can find is that It->x must be equivalent to (*It).x, which is true here.

I double-checked and you're right, this is not a requirement of the STL.

> Also looking at the requirements for forward iterators I think that this iterator should actually be downgraded to an input iterator, because of the requirement that reference = T&.

My concern is that with the less functional iterators, common algorithms get more expensive. For instance, `std::distance()`, `std::advance()` become more expensive without random access, and things like `std::prev()` become impossible.

It seems like the view this needs to provide is a read-only access to the `AssociationTy` objects (because we need to construct them on the fly), but not the data contained within them. If the only thing you can get back from the iterator is a const pointer/reference/value type, then we could store a local "current association" object in the iterator and return a pointer/reference to that. WDYT?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57106