Page MenuHomePhabricator

[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

Unit TestsFailed

TimeTest
1,050 mslibcxx CI Debug iterators > libc++.std/containers/views::range_concept_conformance.compile.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++-tot /home/libcxx-builder/.buildkite-agent/builds/549ad6976107-1/llvm-project/libcxx-ci/libcxx/test/std/containers/views/range_concept_conformance.compile.pass.cpp -v --target=x86_64-unknown-linux-gnu -include /home/libcxx-builder/.buildkite-agent/builds/549ad6976107-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/549ad6976107-1/llvm-project/libcxx-ci/build/generic-debug-iterators/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/549ad6976107-1/llvm-project/libcxx-ci/build/generic-debug-iterators/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/549ad6976107-1/llvm-project/libcxx-ci/libcxx/test/support -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -D_LIBCPP_DEBUG=1 -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/549ad6976107-1/llvm-project/libcxx-ci/build/generic-debug-iterators/projects/libcxx/test/std/containers/views/Output/range_concept_conformance.compile.pass.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -fsyntax-only

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
30

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
24

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