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

[Tomás Longeri via Phabricator via llvm-commits]

tlongeri added a comment.

> 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 the zip iterator 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>)

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136777