Skip to content

Conversation

gogagum
Copy link
Contributor

@gogagum gogagum commented Apr 22, 2025

  • operator[] in iterator facade now returns a reference, because its return type must be the same as dereference type for random_access_iterator concept.
  • add tests for categories checks.
  • change trait to determine transform_iterator reference type.

- operator[] in iterator facade now returns a reference, because its
  return type must be the same as dereference type for
  random_access_iterator concept.
- add tests for categories checks.
- change trait to determine transform_iterator reference type.
@Lastique
Copy link
Member

I'm not sure what problem you're solving, but the reference type may not be an actual reference if the transform function returns an rvalue.

@gogagum gogagum force-pushed the feature/make-transform-iterator-save-category branch 2 times, most recently from cec0cbd to cfbaa35 Compare April 23, 2025 03:14
@gogagum gogagum force-pushed the feature/make-transform-iterator-save-category branch from cfbaa35 to d47f252 Compare April 23, 2025 03:25
@Lastique
Copy link
Member

I don't think I will accept these changes as they change the behavior of iterator_facade in an incompatible way and may break user-defined iterators. As it evidently broke counting_iterator. And I also don't think users should be required to define the operator[] (or other operators) themselves - it's the whole reason iterator_facade exists.

If you want operator[] to return an actual reference, a different approach is needed. iterator_facade needs to know whether returning a reference is safe and only then do that. I'm not sure how to do that other than with some new template parameter or a trait (that defaults to false for backward compatibility).

And finally, I'm not sure why you're changing the iterator category of the transform_iterator, but changing output_iterator_tag to input_iterator_tag seems entirely wrong to me.

@Lastique Lastique closed this Apr 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants