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

Michael Kruse via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 19 15:35:32 PDT 2018


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:
> > > > 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.
Comparison of iterators from different containers is undefined (except: value-initialized iterators). You can also compare their pointers: `List.get() == O.get()`. However, since isl list objects use copy-on-write as an implementation detail, it pointer-equality of two list references (as in: two variables of lists that may or may not contain the same elements) was never well-defined.

IMHO the interesting question is whether the additional definedness/safety (accidentally invalidating an iterator is one of my least favorite kinds of bugs) is worth the potential overhead of reference counter increment/decrement.


================
Comment at: unittests/Support/ISLTools.cpp:24
+  EXPECT_THAT(Sets, testing::UnorderedElementsAre(A, B));
+  isl_ctx_free(Ctx.release());
+}
----------------
philip.pfaffe wrote:
> 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?
We could not decide what the right approach is. Freeing the isl_ctx would mean that `isl::ctx` owns the reference. But in most cases, you are not owning it: E.g. if returned by a get_ctx function to do something else.


Repository:
  rPLO Polly

https://reviews.llvm.org/D48136





More information about the llvm-commits mailing list