[PATCH] D25124: Support rvalue references in enumerate range adapter.

Zachary Turner via llvm-commits llvm-commits at lists.llvm.org
Tue Oct 4 09:21:46 PDT 2016


zturner added inline comments.


> chandlerc wrote in STLExtras.h:35-43
> Put these as members of `enumerate_impl` so that they can directly use `R`? I would alse name them `IteratorT` and `ValueT`.
> 
> But I don't know that you want to define `ValueT` this way. Instead, I would use `std::iterator_traits<IteratorT>::value_type` and `std::iterator_traits<IteratorT>::reference` which seem like the more formal way to extract this information.
> 
> However, this may in turn show that you can't actually nest these yet. But if you want to fix that, I think it will need a much more invasive change. I wouldn't worry with that unless you really want to...

As mentioned, `make_filter_range` also uses `IterOfRange`.  In fact, there was a bug in it before, so putting it here both fixes an existing bug in `make_filter_range` and hopefully ensures that nobody will make this same mistake again.  I only put `ValueOfRange` here as well since `IterOfRange` was already here.

In response to your second point, I tried it out and it doesn't quite work.  In order for this to work I will need to typedef `reference` inside of `enumerator_impl::iterator`.  That might seem obvious, but a cursory glance at the standard does not seem to require that range-for-iterators support `iterator_traits`.  So it would be nice to be able to use `enumerate` (and any other range-adaptors we might invent) with iterators that don't have full support for `iterator_traits`.

https://reviews.llvm.org/D25124





More information about the llvm-commits mailing list