[PATCH] D113511: [mlir] Optimize usage of llvm::mapped_iterator

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added inline comments.


================
Comment at: llvm/include/llvm/ADT/STLExtras.h:310-312
+/// A base type of mapped iterator, that is useful for building derived
+/// iterators that do not need/want to store the map function (as in
+/// mapped_iterator). These iterators must simply provide a `mapElement` method
----------------
rriddle wrote:
> dblaikie wrote:
> > Could this be implemented in mapped_iterator as an empty optimization if the mapping function happens to be stateless?
> I think we can make it work for lambda callable types that capture no state.  For non-lambdas I'm not sure though, given that we don't have a reference to the callable, just its type.
Maybe this (mapped_iterator_base) could be moved to a base class of mapped_iterator, where the latter adds the `getFunction()` API, implements `mapElement()`, and `friend`s mapped_iterator_base to give it access. This would keep the two implementations in sync.

For the EBO, `mapped_iterator` could delegate `getFunction()` to a new type `mapped_iterator_storage`, which avoids storing the function when possible.


================
Comment at: llvm/include/llvm/ADT/STLExtras.h:315
+/// type.
+template <typename DerivedT, typename ItTy, typename ReferenceTy>
+class mapped_iterator_base
----------------
It'd be nice to avoid requiring `ReferenceTy`, using the same declval/decltype trick as above.


================
Comment at: llvm/include/llvm/ADT/STLExtras.h:324
+public:
+  using BaseT = mapped_iterator_base<DerivedT, ItTy, ReferenceTy>;
+
----------------
I think this can just be `using BaseT = mapped_iterator_base;`. Also, is it needed? (see other comment below)


================
Comment at: llvm/unittests/ADT/MappedIteratorTest.cpp:54
+                                          std::vector<int>::iterator, int> {
+    using BaseT::BaseT;
+
----------------
Seems like this would be more clearly written as:
```
lang=c++
using BaseT = CustomMapIterator::mapped_iterator_base;
```
Why expose `BaseT`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113511/new/

https://reviews.llvm.org/D113511