This is an archive of the discontinued LLVM Phabricator instance.

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

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

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ldionne requested changes to this revision.Jun 24 2021, 12:01 PM

This is awesome work, thanks a lot!

libcxx/include/__ranges/concepts.h
73

Is this used anywhere else other than in transform_view? If not, let's put it in transform_view.h only.

libcxx/include/__ranges/transform_view.h
2

You should update the Ranges tracking paper too.

50–52

You could factor this out into a concept __transform_view_constraints and use that to simplify your declaration of the nested iterator type below? Just a suggestion, I find it kind of annoying to duplicate the whole

requires view<_View> && is_object_v<_Fn> &&
           regular_invocable<_Fn&, range_reference_t<_View>> &&
           __referenceable<invoke_result_t<_Fn&, range_reference_t<_View>>>

spaghetti when defining the iterator.

103

Please add a comment to replace by all_t so we don't forget.

103–118

I think Chris is talking about __deduce_iterator_category. But reading __deduce_iterator_category, I don't think it does what we want.

However, we could rewrite as:

template<class _Tp> struct __iterator_concept;
template<random_access_range _Tp> struct __iterator_concept<_Tp> { using type = random_access_iterator_tag; };
template<bidirectional_range _Tp> struct __iterator_concept<_Tp> { using type = bidirectional_iterator_tag; };
// etc...

However, I would rename it to __transform_view_iterator_concept or something along those lines because other views/iterators are going to have different mappings from the concept of the underlying view to the concept they themselves model.

179

You can line break here?

179

Also, as a conforming extension, we can make this conditionally noexcept. (please add test)

279

The noexcept looks right to me (as we just discussed live), however I think what we want instead is to add noexcept(...) as a conforming extension to operator*, and then make this one become noexcept(noexcept(*__i)).

282

Actually, recording my discussion with Zoe: If *__i is a prvalue and we try to std::move from it, the return type will be deduced to T&&. So we're going to bind the prvalue to an rvalue reference, which will be dangling as soon as the function finishes. So the way it is written right now is definitely correct.

libcxx/include/ranges
73

Please update the synopsis to match the current state of implementation.

libcxx/test/std/ranges/range.adaptors/range.transform/base.pass.cpp
23 ↗(On Diff #354055)

Please don't use auto since we're trying to also validate the return type of the function.

Also, we should add a static_assert(std::is_same<decltype(xxx.base()), ...>);. This ensures that e.g. we're not returning a View const& from the base() function, which I could picture being a mistake.

libcxx/test/std/ranges/range.adaptors/range.transform/begin.pass.cpp
28–30 ↗(On Diff #354055)

Can you enclose those into scopes like

{
  std::ranges::transform_view transformView(View{buff}, PlusPlus{});
  assert(transformView.begin().base() == buff);
  assert(*transformView.begin() == 1);
}

etc...

That way, you don't even have to number the variables, and IMO that makes the test easier to read.

Applies here and elsewhere, as you see fit.

libcxx/test/std/ranges/range.adaptors/range.transform/iterator/star.pass.cpp
18 ↗(On Diff #354055)

Please add a specific test.

In particular, I'd like to see a test where the function object returns a T, a T& and a T&&, and a confirmation that it works properly. It's also a great place to test the extension of noexcept we'll apply to it.

The same comment applies to operator[] for testing with different ref-qualified types.

Basically, the idea is that if we somehow messed up the return type of operator*, we should have something that catches it.

This revision now requires changes to proceed.Jun 24 2021, 12:01 PM
zoecarver marked 12 inline comments as done.Jun 24 2021, 3:20 PM
zoecarver added inline comments.
libcxx/include/__ranges/concepts.h
73

Yes, it's used in many views.

zoecarver marked 2 inline comments as done.Jun 24 2021, 3:22 PM
zoecarver updated this revision to Diff 354375.Jun 24 2021, 3:23 PM

Apply review comments.

zoecarver updated this revision to Diff 354376.Jun 24 2021, 3:23 PM

Diff off correct commit. Sorry for the noise.

Minor drive-by comments.

libcxx/include/CMakeLists.txt
46–47

Tangent: This one isn't alphabetized at the moment.

libcxx/include/__ranges/concepts.h
73

FWIW, I think the libc++ style here would be to name it either __maybe_const_t or _MaybeConst, and then stick it in <type_traits>.

(The otherwise-badly-named _Parent and _Base typedefs repeated below are merely mirroring the Standard exposition-only wording; see Parent and Base in https://en.cppreference.com/w/cpp/ranges/transform_view/sentinel . So even though they're awfully ugly, I think _Parent and _Base are probably following the right course of action.)

libcxx/test/std/ranges/range.adaptors/range.transform/iterator/requirments.compile.pass.cpp
1 ↗(On Diff #354376)

This file's name is misspelled.

50 ↗(On Diff #354376)

Concepts pedantry: If you use 1 instead of 0 here, you'll have one extra defense (0 can be a null pointer constant, 1 can't).

zoecarver added inline comments.Jun 24 2021, 5:09 PM
libcxx/include/__ranges/concepts.h
73

If you'd rather me put it in type traits that fine with me. But I want to keep the name the same. All of these exposition only concepts and type traits we do a mechanical transformation of concept-or-trait -> __concept_or_trait. Makes it easy to keep track of things, verify correctness, and reduce duplication.

libcxx/include/__ranges/concepts.h
73

Ah, I see https://eel.is/c++draft/ranges.syn has the exposition-only maybe-const. Weird that cppreference avoids maybe-const and just does the English description e.g. "const V if Const is true, otherwise V." Anyway, LGTM.

libcxx/include/__ranges/transform_view.h
19

alphabetize plz

74–75

Teeechnically... it's impossible for humans to write noexcept-clauses. ;) Could you test locally with an iterator type like this, and report back if it prints OK!?
https://godbolt.org/z/fvxTo4n1v

99

This const is on the wrong line; ditto on line 79 above; and then please rearrange these getters into any reasonable order (e.g. begin, begin const, end, end const). Right now they're in the order begin const, begin, end, end const, which combined with the lack of trailing const keyword, confused the heck out of me.

339

Either a missing & in const __sentinel, or a superfluous const — please decide which.
(On line 345 you went with the missing-& interpretation.)
Grep your codebase today!

libcxx/include/optional
936 ↗(On Diff #354376)

These noexcepts look great to me, but perhaps we should add a simple ~five-line test for them in libcxx/test/libcxx/.

libcxx/test/std/ranges/range.adaptors/range.transform/general.pass.cpp
39 ↗(On Diff #354376)

I don't understand what's going on here with std::bind_front, nor the defaulted function argument func = Fn(), but surely our unit tests should not depend on the behavior of rand().
Please switch to e.g. [](int x) { return x + 1; } and remove the bind and default-function-argument stuff.

45 ↗(On Diff #354376)

Is the point of this test to verify that x is in fact a reference into the original viewed range, and not a copy? If so, this could be more explicit. If not, then again this seems overly confusing and complicated.

50–54 ↗(On Diff #354376)

I assume lines 50-54 were here for polyfill purposes; they can be removed now that enable_view is in master, right?

libcxx/test/support/test_iterators.h
234 ↗(On Diff #354376)

This shouldn't be necessary. Not necessarily a bad idea, but unnecessary.

If we're going to noexcept-qualify our test iterators' operator*, operator->, and operator[], that should be done consistently across all of the test iterator types, and it should be done in a separate PR so we can see what effect it has (if any) on the existing test suite.

zoecarver marked 2 inline comments as done.Jun 25 2021, 11:03 AM
zoecarver added inline comments.
libcxx/test/std/ranges/range.adaptors/range.transform/general.pass.cpp
30 ↗(On Diff #354376)

These have been added. Remove the TODO.

39 ↗(On Diff #354376)

The tests in this file are not testing any one thing in particular. They're meant to be a few general use cases that we might see in the wild. I suppose you're right about random, though. I'll find another function.

libcxx/test/support/test_iterators.h
234 ↗(On Diff #354376)

Why shouldn't this be necessary? We need some iterators with noexcept methods to be able to test the noexceptness of transform_view.

zoecarver marked 3 inline comments as done.Jun 25 2021, 11:19 AM
zoecarver added inline comments.
libcxx/include/__ranges/transform_view.h
19

Held over from when it was named semiregular box.

74–75

Yep, it prints OK!. Want me to add a test like this? I think it should be covered by the noexcept tests, but maybe wouldn't hurt.

99

Yes, it's confusing. Which is worse, how it is now, or having it be in a different order from the standard wording?

zoecarver marked an inline comment as done.

Alphabatize.

Const T -> Const T &.

libcxx/test/support/test_iterators.h
234 ↗(On Diff #354376)

For testing purposes, I recommend using int* as your "contiguous, nothrow iterator" and contiguous_iterator<int*> as your "contiguous, non-nothrow iterator." Sure, the former is not only nothrow but trivial; but as I think you yourself have pointed out on some review a few months ago, it's not possible for us to provide all of the combinatorial explosion of undreamt-of possibilities here.

(Tangent: We can pretend to do so via templates, but that's just adding another combinatorial dimension: we then miss out on the "specifically non-template, etc. etc. iterator." Which makes a difference in Ranges!, because certain things (I'm thinking of pointer_traits) behave differently depending on whether the iterator type is a template specialization or not.

  • I don't yet see a concrete reason to provide either a "specifically random-access, specifically nothrow" iterator (as you propose here) nor a "random-access, specifically non-nothrow" iterator (as we have now in trunk).
  • I don't yet see a concrete reason to provide either a "specifically bidirectional, specifically nothrow" iterator (as you don't propose here) nor a "bidirectional, specifically non-nothrow" iterator (as we have now in trunk). Etc.

Alternatively, could your noexceptness test make use of int* (nothrow) and ThrowingIterator (throwing)? I see ThrowingIterator being used in test/std/ranges/range.access/range.access.begin/begin.pass.cpp already.

Anyway, I'm still not philosophically opposed to nothrow-ing our test iterators, but it should be done consistently and in a separate PR, and it should have some rationale in the commit message about why you're noexcept-ing operator* but not operator++ (etc. etc.) What I am philosophically opposed to, is making ad-hoc changes to the signatures of specific test iterators for the subtle benefit of one particular test you want to write. After all, someone else might have already made them non-noexcept for the subtle benefit of some other test, which you would now be quietly nerfing.

Oh, and if you do pursue the separate-PR-consistently idea, I think it should be noexcept(noexcept(reference(*it_))).

libcxx/include/__ranges/transform_view.h
74–75

Aw, geez... Try again with this version, please. https://godbolt.org/z/v16bds7o1
Did I mention it's impossible for humans to write noexcept-clauses? :D (I accidentally left the noexcept off of my Foo::begin method.)

99

I'm not asking for different order (I think?). Just to move the const up:

constexpr __sentinel<true> end() const
  noexcept(noexcept(ranges::end(__base_)))
  requires range<const _View> &&
zoecarver added inline comments.Jun 29 2021, 10:44 AM
libcxx/include/__ranges/transform_view.h
74–75

OK, it still prints OK! :P

99

I'm not asking for different order (I think?)

See above

and then please rearrange these getters into any reasonable order (e.g. begin, begin const, end, end const). Right now they're in the order begin const, begin, end, end const, which combined with the lack of trailing const keyword, confused the heck out of me.

Anyway...

Just to move the const up:

Will do.

libcxx/test/support/test_iterators.h
234 ↗(On Diff #354376)

For testing purposes, I recommend using int* as your "contiguous, nothrow iterator" and contiguous_iterator<int*> as your "contiguous, non-nothrow iterator."

Done :)

  • Move const.
  • Use View over RandomAccessView for noexcept.
zoecarver marked 5 inline comments as done.Jun 29 2021, 10:45 AM
zoecarver marked 5 inline comments as done.Jun 29 2021, 10:51 AM
zoecarver added inline comments.
libcxx/test/std/ranges/range.adaptors/range.transform/general.pass.cpp
45 ↗(On Diff #354376)

Marking this and the one above as done. I think I commented elsewhere: the point of this test isn't to test any one thing in particular, it's just to have some general function implemented in ways that this might be used in the wild.

50–54 ↗(On Diff #354376)

Unfortunately not yet. We still need to actually use enable_view on main.

zoecarver marked 2 inline comments as done.
  • Remove todo
  • Don't rely on random
  • Rename to 'requirements'
  • Use 1 over 0 in subscript reqs test.
ldionne requested changes to this revision.Jun 29 2021, 12:03 PM
ldionne added inline comments.
libcxx/include/__ranges/transform_view.h
34

Reminder to fix this.

79

Stray whitespace!

87

You added noexcept to several begin() and end() methods, but I don't think they are necessary for your tests anymore if you use declval(), as we discussed offline. For consistency, I wouldn't add those noexcept as conforming extensions.

libcxx/include/optional
909 ↗(On Diff #354560)

Those need a LIBCPP_STATIC_ASSERT(noexcept(...)) test added somewhere in the test suite.

libcxx/test/std/ranges/range.adaptors/range.transform/iterator/arithmetic.pass.cpp
13 ↗(On Diff #354560)

This could say something like transform_view::<iterator>::operatorXXX instead to make it clear it's an exposition only type (applies elsewhere too).

libcxx/test/std/ranges/range.adaptors/range.transform/iterator/iter_move.pass.cpp
38 ↗(On Diff #355295)

You could use std::declval<std::ranges::iterator_t<transform_view>&>() instead of .begin() to avoid having to mark .begin() as noexcept, can't you?

libcxx/test/std/ranges/range.adaptors/range.transform/iterator/star.pass.cpp
1 ↗(On Diff #354560)

Filename: deref.pass.cpp instead?

libcxx/test/std/ranges/range.adaptors/range.transform/iterator/types.pass.cpp
13 ↗(On Diff #355295)

Can you spell out which ones you're expecting?

It's useful when e.g. updating tests after those requirements change, if they do.

libcxx/test/std/ranges/range.adaptors/range.transform/types.h
127 ↗(On Diff #355295)

Maybe call this Increment and use x + 1 instead? It feels a bit weird to use an operation that is typically used to mutate state (in-place increment) in the context of a transform_view.

13 ↗(On Diff #354560)

This is a ContiguousView, not just a view, right?

19 ↗(On Diff #354560)

Why are we making all those operations unconditionally noexcept? I don't think that's part of the spec for a contiguous_range, right? We should keep our archetypes minimal - that's the whole point of archetypes.

This revision now requires changes to proceed.Jun 29 2021, 12:03 PM
zoecarver marked 8 inline comments as done.Jun 29 2021, 4:22 PM
zoecarver updated this revision to Diff 355394.Jun 29 2021, 4:22 PM

Apply Louis' comments.

zoecarver updated this revision to Diff 355399.Jun 29 2021, 4:29 PM

Rebase on Louis' patch.

Quuxplusone added inline comments.
libcxx/include/__ranges/transform_view.h
61–62

My intuition is that you want to switch these around:

[[no_unique_address]] __copyable_box<_Fn> __func_;
_View __base_ = _View();

However, I remember @tcanens having counterintuitive-but-correct intuitions about field ordering in other reviews. It would be useful to write those down as guidelines, somewhere. I volunteer to blog it, if someone can explain it to me until I remember it. :)

74–75

I forget the punch line now. I hypothesize that my original comment dated back to when this begin() had a noexcept-clause, which is now (fortunately) removed.

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
50–51 ↗(On Diff #355399)

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

39 ↗(On Diff #354376)

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

50–54 ↗(On Diff #354376)

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

libcxx/test/std/ranges/range.adaptors/range.transform/iterator/compare.pass.cpp
35 ↗(On Diff #355399)

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
26 ↗(On Diff #355399)

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 ↗(On Diff #354376)

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

libcxx/test/std/ranges/range.adaptors/range.transform/iterator/types.pass.cpp
24 ↗(On Diff #355399)

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.

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.

74–75

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.

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
35 ↗(On Diff #355399)

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
73

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
73

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.