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

[Philip Pfaffe via Phabricator via llvm-commits]

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:
> > > > > 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.
I'll buy the comparison point, but I'm not comfortable relying on copy semantics of isl lists:
- the iterator is a template. I can instantiate it for std::vector.
- it'd mean depending on internal implementation details of isl lists.
- iterator use-after-invalidation is a bug everywhere in the code, and is easily checkable. Valgrind will detect this immediately.


Repository:
  rPLO Polly

https://reviews.llvm.org/D48136