This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement the output_iterator and output_range concepts
ClosedPublic

Authored by ldionne on Jul 23 2021, 1:14 PM.

Diff Detail

Event Timeline

ldionne requested review of this revision.Jul 23 2021, 1:14 PM
ldionne created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2021, 1:14 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne updated this revision to Diff 361323.Jul 23 2021, 1:16 PM

Update RangeStatus paper, and move author to "Various" since they were implemented in various patches by various people.

Quuxplusone added a subscriber: Quuxplusone.

LGTM mod comments.

libcxx/docs/Status/RangesPaper.csv
51

Is iter_value_t in the "dependencies" column? I'd think that things that have been completed don't need to mention their dependencies anymore.
Honestly, things that have been completed don't need to mention their authors anymore; but I know this could be perceived as me complaining about other people getting internet points only because I myself have zero internet points at the moment. ;)

libcxx/include/__iterator/concepts.h
133

As usual, I think it would be nice to use static_cast<_Tp&&>(__t) here, but I don't object if we're not doing that (yet).

libcxx/include/__ranges/concepts.h
96

Might drive-by condense these >>s.

libcxx/include/ranges
71

+2 spaces here to match indentation below

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.output/output_iterator.compile.pass.cpp
51

// Not satisfied when we can't assign a T to the result of *it
would be clearer as re parallel construction with line 39. It took me a while to figure out what was going on here.
Admittedly, that was because I fixated on the void operator++(int) on line 42. Consider making that something like const T *operator++(int) — i.e., you should work in the entire expression *it++ = T(), don't just make *it++ ill-formed by itself.

Mordante accepted this revision.Jul 24 2021, 5:02 AM
Mordante added a subscriber: Mordante.

Thanks for working on this!
LGTM modulo some nits.

libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.output/output_iterator.compile.pass.cpp
47

For consistency with the code above, please add a space after the opening parenthesis.

58

For consistency with the code above, please add a space after the opening parenthesis.

This revision is now accepted and ready to land.Jul 24 2021, 5:02 AM
ldionne marked 7 inline comments as done.Jul 26 2021, 8:04 AM
ldionne added inline comments.
libcxx/include/__iterator/concepts.h
133

We're not. If we do that, we can do it across the library and document it somewhere. I don't want to start doing it as one-offs, since that's just creating inconsistency.

libcxx/include/__ranges/concepts.h
96

Will do in a separate NFC commit.

ldionne updated this revision to Diff 361664.Jul 26 2021, 8:05 AM
ldionne marked 2 inline comments as done.

Address review comments

cjdb added a subscriber: cjdb.Jul 27 2021, 9:25 AM

Thanks for implementing this! It somehow slipped by me, but I have two bits of feedback:

  • please update all iterator concept conformance tests
  • please update all range concept conformance tests