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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

For reference: This is an alternative to https://reviews.llvm.org/D47604

[suggestion] Add unit tests for iterating over isl sets.



================
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();
----------------
Why not `const List &`?


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


================
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); }
----------------
[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?


Repository:
  rPLO Polly

https://reviews.llvm.org/D48136