Page MenuHomePhabricator

[libcxx][ranges] Add `ranges::transform_view`.
ClosedPublic

Authored by zoecarver on May 24 2021, 4:38 PM.

Details

Diff Detail

Unit TestsFailed

TimeTest
770 mslibcxx CI C++03 > libcxx-trunk-shared.libcxx/iterators::contiguous_iterators.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/libcxx/test/libcxx/iterators/contiguous_iterators.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -isystem /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/build/generic-cxx03/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++03 -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 -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/build/generic-cxx03/lib -lc++ -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/build/generic-cxx03/lib -pthread -o /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/build/generic-cxx03/projects/libcxx/libcxx/iterators/Output/contiguous_iterators.pass.cpp.dir/t.tmp.exe
550 mslibcxx CI C++03 > libcxx-trunk-shared.std/algorithms/alg_modifying_operations/alg_copy::copy.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/copy.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -isystem /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/build/generic-cxx03/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++03 -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 -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/build/generic-cxx03/lib -lc++ -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/build/generic-cxx03/lib -pthread -o /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/build/generic-cxx03/projects/libcxx/std/algorithms/alg.modifying.operations/alg.copy/Output/copy.pass.cpp.dir/t.tmp.exe
530 mslibcxx CI C++03 > libcxx-trunk-shared.std/algorithms/alg_modifying_operations/alg_copy::copy_backward.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/copy_backward.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -isystem /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/build/generic-cxx03/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++03 -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 -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/build/generic-cxx03/lib -lc++ -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/build/generic-cxx03/lib -pthread -o /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/build/generic-cxx03/projects/libcxx/std/algorithms/alg.modifying.operations/alg.copy/Output/copy_backward.pass.cpp.dir/t.tmp.exe
530 mslibcxx CI C++03 > libcxx-trunk-shared.std/algorithms/alg_modifying_operations/alg_copy::copy_if.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/copy_if.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -isystem /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/build/generic-cxx03/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++03 -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 -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/build/generic-cxx03/lib -lc++ -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/build/generic-cxx03/lib -pthread -o /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/build/generic-cxx03/projects/libcxx/std/algorithms/alg.modifying.operations/alg.copy/Output/copy_if.pass.cpp.dir/t.tmp.exe
550 mslibcxx CI C++03 > libcxx-trunk-shared.std/algorithms/alg_modifying_operations/alg_copy::copy_n.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/copy_n.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -isystem /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/build/generic-cxx03/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++03 -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 -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/build/generic-cxx03/lib -lc++ -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/build/generic-cxx03/lib -pthread -o /home/libcxx-builder/.buildkite-agent/builds/e41ca863a39b-1/llvm-project/libcxx-ci/build/generic-cxx03/projects/libcxx/std/algorithms/alg.modifying.operations/alg.copy/Output/copy_n.pass.cpp.dir/t.tmp.exe
View Full Test Results (315 Failed)

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Quuxplusone added inline comments.Jun 29 2021, 4:57 PM
libcxx/include/__ranges/transform_view.h
81

Nit: These curly braces make me uneasy, just in general. What would go wrong if you changed all the curly braces throughout to parens? Is it worth doing? (I hope that because we control the overload set of __iterator<true>'s constructor, we can be sure that curly braces are safe here. But if we used parens from the get-go, we wouldn't have to be sure.)

259–263

What's the deal with this commented-out operator<=>?

cjdb requested changes to this revision.Jun 29 2021, 6:22 PM
cjdb added a subscriber: CaseyCarter.
  • Please add a new entry to the module map.
  • Please add iterator and range conformance tests (I think you've already completed some of the iterator conformance tests in a roundabout fashion).
libcxx/include/__ranges/transform_view.h
126–142

This should be in __iterator/type_traits.h (or be renamed to something that's transform_view::iterator-specific).

168

I can't remember, but there might be a technical reason why the standard says !Const and not false. cc @tcanens and @CaseyCarter for fact checking.

259–263

D103478 hasn't been merged yet. @zoecarver can you please add a TODO so we remember to come back and uncomment it ASAP?

libcxx/test/std/ranges/range.adaptors/range.transform/general.pass.cpp
39

If there's a call to bind_front then there needs to be a <functional> inclusion.

50–54

This is already in main. I suspect you need to rebase.

51–52

This smells like a bug. std::vector isn't a view and any attempts to treat it as such are IFNDR.

libcxx/test/std/ranges/range.adaptors/range.transform/iterator/compare.pass.cpp
36

Can you add the tests and just have them commented out for now please?

libcxx/test/std/ranges/range.adaptors/range.transform/iterator/plus_minus.pass.cpp
27

Please add a test checking that (iter + n) - n) == iter and (iter - n) + n) == iter.

libcxx/test/std/ranges/range.adaptors/range.transform/iterator/requirements.compile.pass.cpp
20–32 ↗(On Diff #355399)

Is this not just static_assert(std::bidirectional_iterator<std::ranges::transform_view<BidirectionalView, IncrementConst>>);? Similarly for below with std::random_access_iterator.

libcxx/test/std/ranges/range.adaptors/range.transform/iterator/requirments.compile.pass.cpp
50

We should really be using a variable, not a constant.

libcxx/test/std/ranges/range.adaptors/range.transform/iterator/types.pass.cpp
25

HasIterConcept implies iterator_concept. Perhaps HasIteratorCategory?

libcxx/test/std/utilities/optional/optional.object/optional.object.observe/dereference.pass.cpp
47 ↗(On Diff #355399)

Can you please move the optional changes to their own commit? I'd prefer to keep this one solely focussed on transform_view.

This revision now requires changes to proceed.Jun 29 2021, 6:22 PM
CaseyCarter added inline comments.Jun 29 2021, 7:25 PM
libcxx/include/__ranges/transform_view.h
168

Per https://eel.is/c++draft/class.copy.ctor#5, this constructor declaration is ill-formed when Const is false.

tcanens added inline comments.Jun 29 2021, 8:09 PM
libcxx/include/__ranges/transform_view.h
61–62

After LWG3549 this doesn't really matter.

296

This has been removed by LWG3520.

Quuxplusone added inline comments.Jun 30 2021, 6:45 AM
libcxx/include/__ranges/transform_view.h
61–62

I don't think LWG3549 presents an alternative to view_base for non-libc++ programmers, though, does it? "Civilian" (albeit Niebler-level) programmers are still going to be writing classes that derive from view_base, and so if we're making any layout decisions based on that derivation, our decisions should be unaffected by LWG3549.
I'm thinking of concerns like this:
https://godbolt.org/z/nr9sjbqzn
...Or is the idea that civilians should never use view_base, they should always use view_interface<CRTP> instead? (In that case, view_base probably should never have been specified in the standard?)

tcanens added inline comments.Jun 30 2021, 7:01 AM
libcxx/include/__ranges/transform_view.h
61–62

View adaptors that use view_base are saying "I don't care about the potential extra padding". I don't really care about making a wrapped external thing work better with some other external wrapper - it's trying to save other people from themselves.

view_base is still handy for concrete views (that aren't adaptors).

zoecarver marked 20 inline comments as done.Jul 1 2021, 9:39 AM
zoecarver added inline comments.
libcxx/include/__ranges/transform_view.h
61–62

I've made both of these no_unique_address and flipped the order. This will open us up to problems, though, if someone has an empty view and empty function where the function is copyable and has a copy ctor with side effects. If they use two transform views that are both marked as no_unique_address and try to assign one of them to the other, it won't actually invoke the copy ctor. I think Louis is addressing this in the copyable box patch, though.

73–74

FWIW: this printed OK both with and without the noexcepts.

127

Rename to __transform_view_iterator_category_base.

168

Marking as complete.

296

Perfect :)

libcxx/test/std/ranges/range.adaptors/range.transform/iterator/requirements.compile.pass.cpp
20–32 ↗(On Diff #355399)

I updated based on your suggestion. I think it's kind of nice to spell out the exact requirements for each overload, but this is way clearer and simpler, so I think you're right, it is better.

zoecarver marked 5 inline comments as done.Jul 1 2021, 9:40 AM
zoecarver updated this revision to Diff 355920.Jul 1 2021, 9:41 AM

Update based on Chris' comments.

Quuxplusone added inline comments.Jul 1 2021, 10:30 AM
libcxx/include/__ranges/transform_view.h
61–62

If they use two transform views that are both marked as no_unique_address and try to assign one of them to the other, it won't actually invoke the copy ctor.

I don't understand. Can you show a Godbolt that's as close as possible to what you're talking about? I would also accept a code snippet that displays the problem on libc++-after-this-patch, even if I had to compile it locally.

I wonder if you're talking about the fact that copyable-box defines its assignment operator in terms of a 1990s-flavored if (this != &rhs) test. You're right that that's going to do the wrong thing if we have two distinct copyable-box objects located at the same memory address. However, I think it's probably up to copyable-box to ensure somehow that that doesn't happen (and/or just a QoI issue).

zoecarver added inline comments.Jul 1 2021, 10:39 AM
libcxx/include/__ranges/transform_view.h
61–62

I wonder if you're talking about the fact that copyable-box defines its assignment operator in terms of a 1990s-flavored if (this != &rhs) test. You're right that that's going to do the wrong thing if we have two distinct copyable-box objects located at the same memory address. However, I think it's probably up to copyable-box to ensure somehow that that doesn't happen (and/or just a QoI issue).

Yes, this is what I'm talking about, and yes I think this is copyable-box's problem (see my comment in that review).

I think we're on the same page now, but if not, I can try to find an example.

zoecarver marked an inline comment as done.Jul 1 2021, 10:45 AM
zoecarver added inline comments.
libcxx/test/std/utilities/optional/optional.object/optional.object.observe/dereference.pass.cpp
47 ↗(On Diff #355399)

Done.

ldionne added inline comments.Jul 1 2021, 12:16 PM
libcxx/include/__ranges/transform_view.h
61–62

I concur, I think this is copyable_box's problem. I'll address that over there, and add a test so you can see what we're talking about if you're still unsure.

290

As it stands, I believe the Spec has an issue. CC @tcanens . I think iter_move(transform_view::iterator) is never noexcept. It is defined in http://eel.is/c++draft/range.transform.iterator as:

constexpr decltype(auto) iter_move(const transform_view::iterator& i)
    noexcept(noexcept(invoke(*i.parent_->fun_, *i.current_))) {
    if constexpr (is_lvalue_reference_v<decltype(*i)>)
        return std::move(*i);
    else
        return *i;
}

However, *i.parent_->fun_ dereferences the copyable-box holding the function object, but copyable-box is specified to have the same API as std::optional. In the spec, std::optional::operator* is not noexcept, so the whole noexcept(noexcept(invoke(*i.parent_->fun_, *i.current_))) is always noexcept(false) unless the library provides a noexcept std::optional::operator* as an extension.

@tcanens Does that make sense? Is this LWG issue worthy?

(marking as Done since it's not an issue with this review per se).

libcxx/test/std/ranges/range.adaptors/range.transform/iterator/compare.pass.cpp
36

Or better, #if !defined(_LIBCPP_VERSION), with a TODO comment.

ldionne accepted this revision.Jul 1 2021, 12:20 PM

LGTM with remaining feedback addressed, CI passing and rebased onto the other patches as required (optional and indirectly_swappable). I don't see any blocking issue once the remaining comments have been addressed.

Please wait for @cjdb 's approval before shipping even once you do all of the above, since he had comments.

tcanens added inline comments.Jul 1 2021, 12:42 PM
libcxx/include/__ranges/transform_view.h
290
zoecarver updated this revision to Diff 356016.Jul 1 2021, 2:39 PM
zoecarver marked an inline comment as done.
  • Add tests for spaceship.
cjdb requested changes to this revision.Jul 1 2021, 2:51 PM
cjdb added inline comments.
libcxx/include/__ranges/concepts.h
108

This isn't a range concept (and it's not specific to ranges even if that's the only current client), so I'd put it in <type_traits>. Non-blocking.

libcxx/include/__ranges/transform_view.h
168

Why? It's still ill-formed and needs to be changed to !_Const AIUI.

libcxx/test/std/utilities/optional/optional.object/optional.object.observe/dereference.pass.cpp
47 ↗(On Diff #355399)

I still see optional diffs.

This revision now requires changes to proceed.Jul 1 2021, 2:51 PM
zoecarver marked an inline comment as done.Jul 1 2021, 4:42 PM
zoecarver added inline comments.
libcxx/include/__ranges/concepts.h
108

Sure, I'll move it.

libcxx/include/__ranges/transform_view.h
168

Sorry, I thought @CaseyCarter was saying the opposite.

Anyway, can you find an example where there's an observable difference between using false here and !Const?

Here's an example that uses this ctor. I think the requires disabled the overload so the copy constructor is picked:

std::ranges::transform_view<ContiguousView, Increment> transformView;
auto iter = std::move(transformView).begin();
std::ranges::iterator_t<std::ranges::transform_view<ContiguousView, Increment>> i2(iter);
(void)i2;

I'll add the above snippet as a test for the iterator ctor.

Quuxplusone added inline comments.Jul 1 2021, 5:00 PM
libcxx/include/__ranges/transform_view.h
168

This looks like a Clang bug, going back ages: https://godbolt.org/z/jcaE9hx55
All other compilers agree that you aren't allowed to have a "copy" constructor S(S) that takes by value.
So I don't think libc++ should rely on it.

zoecarver updated this revision to Diff 356048.Jul 1 2021, 5:01 PM
zoecarver marked an inline comment as done.
  • Update based on Chris' comments.
  • Add tests for spaceship.
  • Add iterator ctor tests and move maybe_const.
zoecarver added inline comments.Jul 1 2021, 5:03 PM
libcxx/include/__ranges/transform_view.h
168

Interesting, I see. I thought the requires clause might fix this for us, but it looks like it doesn't (tested on GCC).

I'll fix tomorrow morning.

zoecarver updated this revision to Diff 356262.Jul 2 2021, 1:24 PM
  • Fix GCC bots
zoecarver updated this revision to Diff 356266.Jul 2 2021, 1:27 PM
  • Remove indirectly_swappable.
cjdb accepted this revision.Jul 2 2021, 1:36 PM

LGTM, pending green CI

This revision is now accepted and ready to land.Jul 2 2021, 1:36 PM
zoecarver updated this revision to Diff 356279.Jul 2 2021, 2:28 PM
  • Fix GCC for senintel type.
  • Change comment.
  • Re-format spacing.
zoecarver updated this revision to Diff 357036.Jul 7 2021, 12:04 PM
  • Fix GCC: make current iterator public.
zoecarver updated this revision to Diff 357058.Jul 7 2021, 12:56 PM

Rebase on main.

zoecarver updated this revision to Diff 357090.Jul 7 2021, 3:06 PM
  • Re order when members are initialized in ctor.
zoecarver updated this revision to Diff 357308.Jul 8 2021, 11:50 AM
  • Fix ARM and Windows: replace long with auto
This revision was automatically updated to reflect the committed changes.