[PATCH] D48136: [Polly] Implement an iterator for isl maps, basic_maps, sets, basic_sets

[Philip Pfaffe via Phabricator via llvm-commits]

philip.pfaffe marked an inline comment as done.
philip.pfaffe added inline comments.


================
Comment at: include/polly/Support/ISLTools.h:28
+                           std::forward_iterator_tag, ElementT> {
+  explicit iterator_base(ListT &List) : List(&List) {
+    Position = static_cast<DerivedT *>(this)->list_size();
----------------
Meinersbur wrote:
> Why not `const List &`?
I guess technically it could be `const`, since in isl everything is by-value.


================
Comment at: include/polly/Support/ISLTools.h:50
+protected:
+  ListT *List;
+  int Position = 0;
----------------
Meinersbur wrote:
>  Should this hold a reference to the list? That is, the underlying object is not freed until all iterators are destroyed.
It needs a reference to do the iteration.


================
Comment at: include/polly/Support/ISLTools.h:90
+
+inline map_iterator begin(isl::map_list M) { return map_iterator(M, 0); }
+inline map_iterator end(isl::map_list M) { return map_iterator(M); }
----------------
Meinersbur wrote:
> [suggestion] Make it take `const isl::map_list &M`, so we safe a reference counting cycle.
> 
> [serious] Without having looked further into it, does the `map_iterator` store a pointer to `M`, which does goes out-of-scope after leaving this function?
You're right, this should take a reference to the list.


Repository:
  rPLO Polly

https://reviews.llvm.org/D48136