[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.

A follow-up to last week's phone call:

C++ compilers cannot optimize iterators based on what the equality operator's domain is because the compiler has no understanding of what a iterator is. It is a library concept.

Previous drafts for C++ contained axioms, which could be used by the compiler for optimization (I think they were mostly meant for correctness checking, not optimization, as concepts were also mostly about improving compiler error messages), if such axiom was added to the iterator concept. This is speculative, but I'd assume such a axiom could at most be added to iterator implementations, not iterators in general. Most iterators don't even store a reference/pointer/etc to the container object they are iterating over.

The most recent draft for concepts <http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0734r0.pdf> does not contain axiom or mention that these could be used for compiler optimizations.



================
Comment at: unittests/Support/ISLTools.cpp:24
+  EXPECT_THAT(Sets, testing::UnorderedElementsAre(A, B));
+  isl_ctx_free(Ctx.release());
+}
----------------
philip.pfaffe wrote:
> Meinersbur wrote:
> > 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.
> Then why not just return the C object? Or decide ownership based on how the object is constructed?
@grosser used this for Polly's `isl-noexceptions.h` since `get_ctx()` had to return something. The isl version of it never generated something using `isl_ctx` as parameter or return value. I don't know why he chose to not use `isl_ctx` directly, but I had no objections.


Repository:
  rPLO Polly

https://reviews.llvm.org/D48136