[PATCH] D136777: llvm::zip should not require assignment operator

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D136777#3890093 <https://reviews.llvm.org/D136777#3890093>, @tlongeri wrote:

>> I've got mixed feelings - the mechanics of the change seem fine, but iterator requirements include copy assignability and copy constructibility - so whatever iterator you're going to use with this feature isn't implementing the iterator requirements and probably should be fixed?
>
> I missed this, to be honest. The context is just that I tried and failed to zip MLIR's ElementsAttrIterator <https://mlir.llvm.org/doxygen/classmlir_1_1detail_1_1ElementsAttrIterator.html>, and I saw no reason why zip needed assignment.
>
>> I guess the C++20 iterator concepts are a bit more flexible and may include this situation.
>
> Unfortunately, it looks like the C++20 input_or_output_iterator <https://en.cppreference.com/w/cpp/iterator/input_or_output_iterator> concept still requires //move// assignability (through -> weakly_incrementable <https://en.cppreference.com/w/cpp/iterator/weakly_incrementable> -> movable <https://en.cppreference.com/w/cpp/iterator/weakly_incrementable>)

Yeah - though I think the existing code before your patch here requires copy constructibliity - so this patch does fix llvm::zip to work with these newer iterator concepts, which is good.

> Isn't it still a small improvement, though? Besides what Mehdi said, I would just add that it still seems like incrementing the iterators in-place is a bit better. Maybe I should adjust the description, though.

Yep - might be worth checking the ElementsAttrIterator to ensure it at least meets the newer iterator concepts at least. (my usual concern here is not that changing one algorithm to be more lenient - but that a non-conforming iterator will run up against other problems, standard algorithms will have difficult/might have more stringent checks, etc).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136777