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

Philip Pfaffe via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 19 02:15:59 PDT 2018


philip.pfaffe added inline comments.


================
Comment at: include/polly/Support/ISLTools.h:50
+protected:
+  ListT *List;
+  int Position = 0;
----------------
Meinersbur wrote:
> 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.
If I capture the List by value, how do I compare iterators? I'll have to compare the lists by value then, and that's quite expensive.

I can certainly document iterator invalidation, but it behaves just as you'd expect: destruction or modification of the list invalidate.


================
Comment at: unittests/Support/ISLTools.cpp:24
+  EXPECT_THAT(Sets, testing::UnorderedElementsAre(A, B));
+  isl_ctx_free(Ctx.release());
+}
----------------
Meinersbur wrote:
> [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);
> ```
Why doesn't the C++ wrapper do this already?


Repository:
  rPLO Polly

https://reviews.llvm.org/D48136





More information about the llvm-commits mailing list