This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][iterator][ranges] adds `bidirectional_iterator` and `bidirectional_range`
ClosedPublic

Authored by cjdb on Apr 11 2021, 6:19 PM.

Details

Summary

Implements parts of:

  • P0896R4 The One Ranges Proposal`

Depends on D100275.

Diff Detail

Event Timeline

cjdb requested review of this revision.Apr 11 2021, 6:19 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2021, 6:19 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cjdb updated this revision to Diff 337044.Apr 12 2021, 10:01 PM

updates subsumption test

cjdb updated this revision to Diff 339420.Apr 21 2021, 3:57 PM

rebases so @zoecarver isn't blocked

This patch is not ready for another round of reviews

cjdb updated this revision to Diff 340367.Apr 25 2021, 9:59 AM
cjdb retitled this revision from [libcxx] adds `bidirectional_iterator` and `bidirectional_range` to [libcxx][iterator][ranges] adds `bidirectional_iterator` and `bidirectional_range`.
  • moves conformance tests to respective files
  • adds canonical test type cxx20_bidirectional_iterator
cjdb added a subscriber: cor3ntin.Apr 29 2021, 10:37 AM
ldionne requested changes to this revision.Apr 29 2021, 12:16 PM

Mostly LGTM, with minor nits.

libcxx/test/std/containers/sequences/array/range_concept_conformance.compile.pass.cpp
29

Shouldn't we keep both the assertions for forward_range and bidirectional_range? (here and everywhere else)

libcxx/test/support/test_iterators.h
699 ↗(On Diff #340367)

To remove IIUC.

This revision now requires changes to proceed.Apr 29 2021, 12:16 PM
ldionne added inline comments.Apr 29 2021, 12:21 PM
libcxx/test/std/iterators/iterator.requirements/iterator.concepts/iterator.concept.bidir/bidirectional_iterator.compile.pass.cpp
24

I love those tests, BTW, very simple and they test just what needs to be tested.

cjdb updated this revision to Diff 341747.Apr 29 2021, 7:34 PM
cjdb marked an inline comment as done.

rebases to activate CI

cjdb updated this revision to Diff 341959.Apr 30 2021, 10:02 AM

rebases to activate CI

cjdb updated this revision to Diff 342087.Apr 30 2021, 4:52 PM
  • updates iterator conformance tests
  • moves subsumption test from test/std/ to test/libcxx/
cjdb updated this revision to Diff 342098.Apr 30 2021, 5:53 PM

rebases to activate CI

cjdb updated this revision to Diff 342460.May 3 2021, 10:39 AM

removes XFAIL

ldionne accepted this revision.May 3 2021, 11:19 AM

Chris and I had a chat offline and we agreed that the derived_from part of the subsumption tests can be removed, and so the tests moved back to test/std. LGTM with those changes in.

libcxx/test/libcxx/iterators/iterator.concepts/iterator.concept.bidir/subsumption.compile.pass.cpp
23 ↗(On Diff #342460)

This is a nit, but I don't feel like adding [[nodiscard]] here adds anything. I would remove it here (and in all other places where we added it in similar places).

I think we need to clarify libc++'s policy for [[nodiscard]] to avoid marking every single function with it, since that gets into diminishing returns and actually makes stuff harder to read. You can check this in as-is though, and I'll make a NFC commit that changes it and other instances.

This revision is now accepted and ready to land.May 3 2021, 11:19 AM