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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
