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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: include/polly/Support/ISLTools.h:50
+protected:
+  ListT *List;
+  int Position = 0;
----------------
philip.pfaffe wrote:
> Meinersbur wrote:
> > philip.pfaffe wrote:
> > > 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.
> > What I mean is that the iterator could increase isl's ref counter to ensure that it is not free'd as long as one iterator exists. That is, is the following code valid?
> > ```
> > basic_set_iterator I, E;
> > {
> >   auto List = S.get_basic_set_list();
> >   I = begin(List); E = end(List);
> > } // List out-of-scope here
> > for (; I != E; ++I) { ... }
> > ```
> Well, if you destroy the container you obtained iterators for and then use the iterators, that's on you ;)
Using reference counting would allow us to not invalidate the iterators.

It's not just destroying the container, but also modifying it while iterating. Isl objects are reference counted, so the underlying object might be shared by other references as well.

At least it should be documented when iterators are invalidated.


================
Comment at: unittests/Support/ISLTools.cpp:24
+  EXPECT_THAT(Sets, testing::UnorderedElementsAre(A, B));
+  isl_ctx_free(Ctx.release());
+}
----------------
[suggestion] I quite like using RAII to free the `isl_ctx` as done in `IslTest.cpp`:
```
std::unique_ptr<isl_ctx, decltype(&isl_ctx_free)> Ctx(isl_ctx_alloc(), &isl_ctx_free);
```


Repository:
  rPLO Polly

https://reviews.llvm.org/D48136