[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:315
+/// type.
+template <typename DerivedT, typename ItTy, typename ReferenceTy>
+class mapped_iterator_base
----------------
rriddle wrote:
> dexonsmith wrote:
> > It'd be nice to avoid requiring `ReferenceTy`, using the same declval/decltype trick as above.
> I'm not sure we can here, given that we don't have the exact function type, and I don't think we can resolve `DerivedT::mapElement` at this point? I could be wrong here.
No, I think you're right.


================
Comment at: llvm/unittests/ADT/MappedIteratorTest.cpp:54
+                                          std::vector<int>::iterator, int> {
+    using BaseT::BaseT;
+
----------------
rriddle wrote:
> dexonsmith wrote:
> > Seems like this would be more clearly written as:
> > ```
> > lang=c++
> > using BaseT = CustomMapIterator::mapped_iterator_base;
> > ```
> > Why expose `BaseT`?
> This is inheriting the constructor of the base class.
> 
> > Why expose BaseT?
> I've gotten into a habit of using BaseT to avoid the need to have long constructor  inheritance lines, though that probably isn't necessary here given that you've pointed out that we don't need the super long template params (thanks for the comment on the other git commit BTW, need to clean other uses of this in MLIR. I wasn't sure if that was allowed on all of the compilers we support).
> This is inheriting the constructor of the base class.

Right, thanks, I misread it as exposing the typedef :).

> > Why expose BaseT?
> I've gotten into a habit of using BaseT to avoid the need to have long constructor  inheritance lines, though that probably isn't necessary here given that you've pointed out that we don't need the super long template params (thanks for the comment on the other git commit BTW, need to clean other uses of this in MLIR. I wasn't sure if that was allowed on all of the compilers we support).

Yup, figured that was the reason; I only realized we could do this myself when I saw it in `iterator_adaptor_base` recently (I was thinking of adding a `BaseT`...).

I didn't intend to be oblique; wondered if there was some generic algorithm relying on the `BaseT` typedef and it seemed like this test was ensuring it worked (due to misreading the inherited constructor).

(IMO we should skip these now where possible now that we know we can (the verbose thing is more clear and avoids the extra ad-hoc typedef), but not a big deal if you'd prefer to keep it for some reason.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113511