Page MenuHomePhabricator

[libcxx][ranges] Add `std::ranges::single_view`.
ClosedPublic

Authored by zoecarver on Jul 26 2021, 5:28 PM.

Details

Reviewers
cjdb
ldionne
Group Reviewers
Restricted Project
Commits
rG481ad59b9fa4: [libcxx][ranges] Add `std::ranges::single_view`.

Diff Detail

Unit TestsFailed

TimeTest
870 mslibcxx CI GCC 11 / C++latest > libc++.std/ranges/range_factories/range_single_view::assign.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-11 /home/libcxx-builder/.buildkite-agent/builds/c6aa01ffc3f8-1/llvm-project/libcxx-ci/libcxx/test/std/ranges/range.factories/range.single.view/assign.pass.cpp -v -include /home/libcxx-builder/.buildkite-agent/builds/c6aa01ffc3f8-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/c6aa01ffc3f8-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/c6aa01ffc3f8-1/llvm-project/libcxx-ci/build/generic-gcc/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/c6aa01ffc3f8-1/llvm-project/libcxx-ci/libcxx/test/support -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-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -lc++experimental -L/home/libcxx-builder/.buildkite-agent/builds/c6aa01ffc3f8-1/llvm-project/libcxx-ci/build/generic-gcc/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/c6aa01ffc3f8-1/llvm-project/libcxx-ci/build/generic-gcc/./lib -L/home/libcxx-builder/.buildkite-agent/builds/c6aa01ffc3f8-1/llvm-project/libcxx-ci/build/generic-gcc/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/c6aa01ffc3f8-1/llvm-project/libcxx-ci/build/generic-gcc/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/c6aa01ffc3f8-1/llvm-project/libcxx-ci/build/generic-gcc/projects/libcxx/test/std/ranges/range.factories/range.single.view/Output/assign.pass.cpp.dir/t.tmp.exe
920 mslibcxx CI GCC 11 / C++latest > libc++.std/ranges/range_factories/range_single_view::ctor.in_place.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-11 /home/libcxx-builder/.buildkite-agent/builds/c6aa01ffc3f8-1/llvm-project/libcxx-ci/libcxx/test/std/ranges/range.factories/range.single.view/ctor.in_place.pass.cpp -v -include /home/libcxx-builder/.buildkite-agent/builds/c6aa01ffc3f8-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/c6aa01ffc3f8-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/c6aa01ffc3f8-1/llvm-project/libcxx-ci/build/generic-gcc/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/c6aa01ffc3f8-1/llvm-project/libcxx-ci/libcxx/test/support -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-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -lc++experimental -L/home/libcxx-builder/.buildkite-agent/builds/c6aa01ffc3f8-1/llvm-project/libcxx-ci/build/generic-gcc/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/c6aa01ffc3f8-1/llvm-project/libcxx-ci/build/generic-gcc/./lib -L/home/libcxx-builder/.buildkite-agent/builds/c6aa01ffc3f8-1/llvm-project/libcxx-ci/build/generic-gcc/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/c6aa01ffc3f8-1/llvm-project/libcxx-ci/build/generic-gcc/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/c6aa01ffc3f8-1/llvm-project/libcxx-ci/build/generic-gcc/projects/libcxx/test/std/ranges/range.factories/range.single.view/Output/ctor.in_place.pass.cpp.dir/t.tmp.exe…

Event Timeline

zoecarver created this revision.Jul 26 2021, 5:28 PM
zoecarver requested review of this revision.Jul 26 2021, 5:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 26 2021, 5:28 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cjdb requested changes to this revision.Jul 26 2021, 6:29 PM

Please add concept conformance tests!

libcxx/test/std/ranges/range.factories/range.single.view/assign.pass.cpp
15

I think these should be backticks, not angle brackets.

libcxx/test/std/ranges/range.factories/range.single.view/begin.pass.cpp
72

Commented out.

This revision now requires changes to proceed.Jul 26 2021, 6:29 PM
zoecarver marked 2 inline comments as done.Jul 27 2021, 9:27 AM
zoecarver added inline comments.
libcxx/test/std/ranges/range.factories/range.single.view/assign.pass.cpp
15

We decided to use angle brackets for the exposition only types.

libcxx/test/std/ranges/range.factories/range.single.view/begin.pass.cpp
72

Good catch, thanks.

zoecarver updated this revision to Diff 362066.Jul 27 2021, 9:27 AM
zoecarver marked 2 inline comments as done.

Apply Chris' feedback.

cjdb added a comment.Jul 27 2021, 9:29 AM

Nearly there!

libcxx/test/std/ranges/range.factories/range.single.view/view_conformance.compile.pass.cpp
1

Please rename file to match the other range concept conformance tests.

25

This is a test for single_view, no?

ldionne requested changes to this revision.Jul 28 2021, 7:36 AM
ldionne added inline comments.
libcxx/include/__ranges/copyable_box.h
182

_VSTD::addressof(__val_)

Also, would it be possible to add a test for it, like we do for operator*?

libcxx/include/__ranges/single_view.h
1

Could you add views::single at the same time? It's pretty small so it makes sense to implement both in the same go, I think.

Also, this makes me think that we don't have views::empty, right?

40

Here you could use __value_(std::in_place, __t) instead, and you would not have to modify __copyable_box. Same for the rvalue ref below.

libcxx/test/std/ranges/range.factories/range.single.view/size.pass.cpp
13

Since the spec documents this as being static, can you add a test that std::ranges::single_view<int>::size() works as well?

This revision now requires changes to proceed.Jul 28 2021, 7:36 AM
cjdb added inline comments.Jul 28 2021, 9:13 AM
libcxx/include/__ranges/single_view.h
1

If you're going to take @ldionne up on this (and I think you should), please DM me so we can discuss a cohesive design for the adaptor closures. I don't want us to have wildly different ideas about what goes in this namespace.

libcxx/test/std/ranges/range.factories/range.single.view/size.pass.cpp
13

Unless there are already tests in ranges::size for static members, is it worth adding a test here for ranges::size too?

zoecarver marked 2 inline comments as done.Jul 28 2021, 11:24 AM
zoecarver added inline comments.
libcxx/test/std/ranges/range.factories/range.single.view/view_conformance.compile.pass.cpp
25

🤦‍♂️

zoecarver marked an inline comment as done.

Address Chris' comments.

zoecarver marked 4 inline comments as done.

Address Louis' comments.

ldionne accepted this revision.Jul 28 2021, 12:19 PM

LGTM with all comments applied and CI passing.

libcxx/include/__ranges/single_view.h
1

views::single is not a range adaptor object, so we still are not implementing the pipe operator. But regardless, let's chat on Discord since you seem to have a specific approach in mind for the pipe operator.

libcxx/test/std/ranges/range.factories/range.single.view/assign.pass.cpp
2

Stray comment?

11

I really like that you're testing this property of the type, thanks for being thorough.

libcxx/test/std/ranges/range.factories/range.single.view/size.pass.cpp
13

I'm not sure I understand your comment. Are you asking that we should add a test for ranges::size() on a type that provides a static size() method? If that's your suggestion, I would agree. It's actually a hole in our current testing coverage, since the spec for ranges::size() basically says "use x.size() if that is valid", which does not specify whether .size() is a static or non-static member function.

So as a fly-by fix in this patch, @zoecarver can you add a test in ranges::size() with a type that defines a static size() function?

zoecarver marked 2 inline comments as done.Jul 28 2021, 3:38 PM

So it appears that the assign test crashes on GCC trunk! Anyway, I filed a bug (id = 101663), and I'll mark this test as unsupported on GCC.

libcxx/test/std/ranges/range.factories/range.single.view/assign.pass.cpp
11

Thank you :)

zoecarver marked 2 inline comments as done.Jul 28 2021, 3:38 PM
zoecarver updated this revision to Diff 362570.Jul 28 2021, 3:39 PM

Apply review feedback

cjdb accepted this revision.Jul 28 2021, 4:07 PM

LGTM pending last bit of feedback (just test it locally, no need to wait for CI).

libcxx/test/std/ranges/range.factories/range.single.view/range_concept_conformance.compile.pass.cpp
25 ↗(On Diff #362570)

This should be input_range.

This revision is now accepted and ready to land.Jul 28 2021, 4:07 PM
zoecarver updated this revision to Diff 362580.Jul 28 2021, 4:18 PM

Apply Chris' last comment. Fix GCC.

This revision was automatically updated to reflect the committed changes.