[PATCH] D48348: [ADT] Add zip_longest iterators.

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

In D48348#1315100 <https://reviews.llvm.org/D48348#1315100>, @Meinersbur wrote:

> In D48348#1310674 <https://reviews.llvm.org/D48348#1310674>, @dblaikie wrote:
>
> > I /think/ this still doesn't meet the iterator requirements (that if "(*i).j" is valid, then "i->j" must be valid too - [input.iterators]p2, Table 95, "a->m is equivalent to (*a).m if a is dereferencable"). I believe the way standard iterators (like istream_iterator) implement this is by having an instance of the value type inside the iterator that has the current value in it, so operator-> can return a pointer to that. Would that be possible/practical?
>
>
> The type of `*a` is a `std::tuple` of the client-iterator's value types. Storing an internal scratch field inside the the zip-operator would allow `operator->` to return a pointer. However, `std::tuple` has just two methods that could be called: `swap` and `operator=`. Both would be useless and `operator->` should probably return a `const std::tuple*` to avoid messing with the internal field anyway. It still would not allow assigning to the tuple's elements.
>
> Experimenting with `llvm::zip` and `llvm::map_iterator` shows that they don't meet the requirement either:
>
>   const SmallVector<unsigned, 6> pi{3, 1, 4, 1, 5, 9};
>   auto i = map_iterator(pi.begin(), [](unsigned v) -> std::pair<unsigned,char> {return (v == 3) ? std::make_pair(v,'a') : std::make_pair(v,'\0');} );
>   (*i).first;
>   i->first;
>  
>   error: taking the address of a temporary object of type 'std::pair<unsigned int, char>' [-Waddress-of-temporary]
>     PointerT operator->() { return &static_cast<DerivedT *>(this)->operator*(); }
>                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>
>
>
>         SmallVector<unsigned, 6> pi{3, 1, 4, 1, 5, 9};
>         auto i = zip(pi,pi).begin();
>         std::tuple<const unsigned&,const unsigned&> x{pi[0],pi[1]};
>         (*i).operator=(x);
>         i->operator=(x);
>  
>   llvm/ADT/iterator.h:170:34: error: taking the address of a temporary object of type 'llvm::detail::zip_common<llvm::detail::zip_shortest<unsigned int *, unsigned int *>, unsigned int *, unsigned int *>::value_type' (aka 'std::tuple<unsigned int &, unsigned int &>') [-Waddress-of-temporary]
>     PointerT operator->() { return &static_cast<DerivedT *>(this)->operator*(); }
>                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>
> To answer the question. Yes, it it possible to add a scratch `std::tuple` to `zip_longest` iterators, but there is no practical gain.


Fair enough - thanks for the explanation (it should be const and there are no const member functions to use, so there's no "i->u" or "(*i).u" that's valid to support).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D48348