Page MenuHomePhabricator

[libcxx][ranges] adds _`copyable-box`_
ClosedPublic

Authored by ldionne on May 9 2021, 10:11 AM.

Details

Summary

Implements part of P2325.
Implements [range.copy.wrap].

Depends on D102119.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
zoecarver added inline comments.Jun 29 2021, 3:38 PM
libcxx/include/__ranges/copyable_box.h
52

Ah, I see you already explained this: https://reviews.llvm.org/D102135#inline-994703

FWIW I don't find that too weird and wouldn't be opposed to inheriting here. Especially given this is an exposition only type.

Either way works for me.

Quuxplusone requested changes to this revision.Jun 29 2021, 3:59 PM
Quuxplusone added inline comments.
libcxx/include/__ranges/copyable_box.h
52
57

I'm skeptical of the use of a user-visible type like in_place_t here, for basically the same reason I'm skeptical of the operator bool. However, in this case I have no particular better suggestion.

64

This comment is either wrong (I hope) or horrible. If it's true that parens wouldn't work here... why not? That's what the comment should be explaining.

92

I'd rather see return __val_.has_value(); — it's shorter and also easier to understand what it's doing.

libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/assign.copy.pass.cpp
45

Should we test that this (does | doesn't) end up calling CopyConstructible::operator=?

114–115

It would still be nice to move this chunk of tests into a helper function, e.g. void test_empty_state().

libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/assign.move.pass.cpp
23

It'd be worth unifying the 2 different approaches to this diagnostic in our tests. Either way, don't add a 3rd.

test/std/input.output/filesystems/class.directory_iterator/directory_iterator.members/move_assign.pass.cpp:#pragma clang diagnostic ignored "-Wself-move"
test/std/input.output/filesystems/class.rec.dir.itr/rec.dir.itr.members/move_assign.pass.cpp:#pragma clang diagnostic ignored "-Wself-move"
test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.asgn/move.pass.cpp:// ADDITIONAL_COMPILE_FLAGS: -Wno-self-move
libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/operator-bool.pass.cpp
24–25 ↗(On Diff #355358)

Perhaps also test

static_assert(!std::is_convertible_v<decltype(x), bool>);

However, I have a higher-level question: Why on earth do we want this internal implementation detail type to be contextually convertible to bool in the first place?! It should just have a .has_value() member, and we should use x.has_value() everywhere we're testing whether it has a value. I don't see any situation where we'd be like "ooh, I wish I could just omit the .has_value part of this expression and it would still compile and do the same thing." For our internal detail types, we have the luxury of getting to design their APIs ourselves, and we don't need to build in that kind of footgun if we don't want to.

libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/types.h
14

I'd rather see just #include <ranges> here.

42

It'd be useful to add static_assert(!std::copyable<NothrowCopyConstructible>); etc. — whatever you expect to be true of this type — right after its closing };. Ditto for the other types in this file. Ideally those static_asserts will help the reader see what the intended differences between these types are.

This revision now requires changes to proceed.Jun 29 2021, 3:59 PM
zoecarver added inline comments.Jun 29 2021, 4:28 PM
libcxx/include/__ranges/copyable_box.h
90

This and the other's need to be noexcept. (After my patch, std::optional's star will also be noexcept.)

Quuxplusone added inline comments.Jun 29 2021, 4:39 PM
libcxx/include/__ranges/copyable_box.h
90

Since this is an internal helper, it doesn't need to be noexcept; there's definitely nothing in libc++ (and by definition, nothing outside libc++) that would ever check its noexceptness-status.
We know from other reviews that there is a danger in accidentally saying noexcept when the function might throw (namely, rogue calls to std::terminate), whereas there is no danger in accidentally saying nothing when the function doesn't throw.
However, I wouldn't strongly object to marking it noexcept, because it's so trivial that its noexceptness is "obvious" (famous last words).

ldionne marked 16 inline comments as done.Thu, Jul 1, 9:28 AM
ldionne added inline comments.
libcxx/include/__ranges/copyable_box.h
90

@zoecarver I'm not sure I understand why it needs to be noexcept. Can you please explain?

125

Good point. The reason why I wrote it that way is that I originally had two specializations, one for copyable and one for is_nothrow_xxx_constructible. I then merged the two implementations somewhat mechanically.

However, I gave more thought and here's what I'm doing now:

template<class _Tp>
concept __doesnt_need_empty_state_for_copy = copyable<_Tp> || is_nothrow_copy_constructible_v<_Tp>;

template<class _Tp>
concept __doesnt_need_empty_state_for_move = movable<_Tp> || is_nothrow_move_constructible_v<_Tp>;

template<__copy_constructible_object _Tp>
  requires __doesnt_need_empty_state_for_copy<_Tp> && __doesnt_need_empty_state_for_move<_Tp>
class __copyable_box<_Tp> {
  // Implementation of assignment operators in case we perform optimization (1)
  constexpr __copyable_box& operator=(__copyable_box const&) requires copyable<_Tp> = default;
  constexpr __copyable_box& operator=(__copyable_box&&) requires movable<_Tp> = default;

  // Implementation of assignment operators in case we perform optimization (2)
  constexpr __copyable_box& operator=(__copyable_box const& __other) noexcept { ... }
  constexpr __copyable_box& operator=(__copyable_box&& __other) noexcept { ... }
};

It's somewhat of a mouthful, but I feel like it expresses what we're trying to do most clearly.

libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/assign.copy.pass.cpp
45

Yes, I think we should. Note that we don't need to test that we don't call operator= in the cases that we don't, since operator= is marked as deleted in those cases.

libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/operator-bool.pass.cpp
24–25 ↗(On Diff #355358)

Hmm, I guess I agree. I added __has_value() and removed operator bool.

libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/properties.compile.pass.cpp
38

Actually, I think we do care about pinning this down for ABI purposes. If our implementation changed somehow, we'd want to make sure that we don't break the ABI, and this is one step towards that. I'll strengthen this to sizeof(copyable-box) == sizeof(std::optional<T>).

libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/types.h
14

Can you explain why? Does that apply to the other test files too?

I like including <__ranges/copyable_box.h> since it's an implementation-detail type, and it has the added benefit that it tests that the header can be included on its own.

ldionne updated this revision to Diff 355911.Thu, Jul 1, 9:29 AM
ldionne marked 4 inline comments as done.

Update per review feedback. Thanks for all the feedback so far!

zoecarver added inline comments.Thu, Jul 1, 10:02 AM
libcxx/include/__ranges/copyable_box.h
90

We need this to be noexcept so that it can be used in transform_view. For example transform_view::operator* needs to be able to dereference the copyable-box to invoke the function object.

112

What do you think about this problem: https://reviews.llvm.org/D103056#inline-999709

Basically, if we have two copyable-box members that are both no_unique_address where T is copyable and empty, we will never invoke the copy constructor on assignment. I think this is probably a bug with copyable box, and not its users.

Quuxplusone added inline comments.Thu, Jul 1, 10:31 AM
libcxx/include/__ranges/copyable_box.h
90

So it needs to be dereferenceable, but why does it need to be noexcept-dereferenceable?
I had a vague notion that maybe we needed it to be noexcept because transform_view::iterator::operator* was specified as "expression-equivalent to" some expression that would be noexcept if not for the noexcept(false)-ness of this function. However, eel.is provides evidence against that hypothesis: https://eel.is/c++draft/range.transform.iterator

constexpr decltype(auto) operator*() const {
  return invoke(*parent_->fun_, *current_);
}

Here the Standard gives a reference implementation (not an "expression-equivalent to"), and the reference implementation is explicitly non-noexcept.

If the Standard had instead depicted

constexpr decltype(auto) operator*() const noexcept(noexcept(invoke(*parent_->fun_, *current_))) {
  return invoke(*parent_->fun_, *current_);
}

then I would agree, marking copyable_box::operator* as noexcept would be the path of least resistance. But it doesn't say that.

Quuxplusone added inline comments.Thu, Jul 1, 10:39 AM
libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/types.h
14

(A) I guess I'm neutral now. If you do <ranges>, then the test is more future-proof against future renamings of internal headers; but of course it's deliberately not future-proof against future renamings of internal types, so maybe who cares. I do believe we both agree that we don't want to see arbitrary test cases including detail headers, though, right? If we keep the detail header here, we're saying it's a special case because we're specifically testing __copyable_box, not setting a precedent for including detail headers in test code.

(B) "tests that the header can be included on its own" — I recently had a shower thought that I ought to write a little Python script to list all the detail headers and for each of them, try compiling a one-line TU including just that header, to help ensure IWYU hygiene. I might even do that this weekend. But I don't think there's any way to bolt such a script onto our actual test suite; it'd just be a one-off local experiment. (Better would probably be to run the codebase through one of those IWYU-enforcing analysis tools, right? I have no experience with such tools.)

tcanens added inline comments.Thu, Jul 1, 12:41 PM
libcxx/include/__ranges/copyable_box.h
112

no_unique_address doesn't change the basic object model rule that two living objects of the same type must have distinct addresses.

ldionne marked 7 inline comments as done.Thu, Jul 1, 12:59 PM
ldionne added inline comments.
libcxx/include/__ranges/copyable_box.h
90

It's for iter_move, which is specified as:

friend constexpr decltype(auto) iter_move(const 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;
}
112

@zoecarver Yup, I thought we were right with the discussion we had about this yesterday, but it was all FUD. As usual, what @tcanens said is correct. I don't think there's any issue here, after all. I'm adding a test that validates it (it's somewhat superfluous though cause we're basically testing that the compiler does the right thing when it sees [[no_unique_address]]).

libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/types.h
14

I do believe we both agree that we don't want to see arbitrary test cases including detail headers, though, right?

Except when what we're testing is precisely those internal "details", and the tests live in libcxx/test/libcxx. In those cases I think it makes more sense to include the header that contains what we're testing than some other header which we know will transitively include it.

ldionne updated this revision to Diff 355992.Thu, Jul 1, 12:59 PM
ldionne marked 3 inline comments as done.

Update per review comments.

zoecarver accepted this revision.Thu, Jul 1, 2:50 PM

LGTM.

libcxx/include/__ranges/copyable_box.h
90

We decided in the review that we wanted operator* to have the correct noexceptness to implement iter_move and for QoI.

112

I don't think there's any issue here, after all. I'm adding a test that validates it (it's somewhat superfluous though cause we're basically testing that the compiler does the right thing when it sees [[no_unique_address]]).

I don't think a test is necessary either, but it doesn't hurt.

By the way, I tested this locally with D103056 and it works fine.

tcanens added inline comments.Thu, Jul 1, 9:08 PM
libcxx/include/__ranges/copyable_box.h
76

It's possible for a movable type to use the primary template, so it also needs a

__copyable_box& operator=(__copyable_box&& __other) requires movable<_Tp> = default;
131–132

The constexpr should be removed - on defaulted functions, an explicit constexpr is an assertion of constexpr-ness.

tcanens added inline comments.Thu, Jul 1, 9:17 PM
libcxx/include/__ranges/copyable_box.h
138–139
147–148
Quuxplusone added inline comments.Fri, Jul 2, 8:14 AM
libcxx/include/__ranges/copyable_box.h
131–132

That is true for defaulted comparison and relational operators in C++20; it's never been true for defaulted special member functions. (And yes this is horrible and I'm pretty sure a defect report has been filed somewhere.)
https://godbolt.org/z/61e564vra

So this code is technically correct as written. But stylistically I agree: writing constexpr here is redundant and should be removed.

Quuxplusone added inline comments.Fri, Jul 2, 8:18 AM
libcxx/include/__ranges/copyable_box.h
90

For the record, the offending iter_move has been eliminated by LWG3520.
However, I don't object to marking copyable-box::operator* as noexcept, just for QoI.

tcanens added inline comments.Fri, Jul 2, 8:19 AM
libcxx/include/__ranges/copyable_box.h
131–132

That is true for defaulted comparison and relational operators in C++20; it's never been true for defaulted special member functions. (And yes this is horrible and I'm pretty sure a defect report has been filed somewhere.)
https://godbolt.org/z/61e564vra

That's not what https://timsong-cpp.github.io/cppwp/dcl.fct.def.default#3.sentence-1 says.

Quuxplusone added inline comments.Fri, Jul 2, 8:29 AM
libcxx/include/__ranges/copyable_box.h
131–132

Hm. That sentence dates back at least to the beginning of the C++11 repository
https://github.com/cplusplus/draft/blob/d8fb8cf16dd05f711d33b3749a757d69068e45ef/source/declarators.tex#L2227-L2229
and yet no compiler has ever implemented it.
https://godbolt.org/z/3TozqePYx
Strange.

tcanens added inline comments.Fri, Jul 2, 12:43 PM
libcxx/include/__ranges/copyable_box.h
90

3520 didn't remove iter_move.

ldionne marked 11 inline comments as done.Mon, Jul 5, 10:06 AM
ldionne added inline comments.
libcxx/include/__ranges/copyable_box.h
131–132

TIL!

ldionne updated this revision to Diff 356523.Mon, Jul 5, 10:06 AM
ldionne marked an inline comment as done.

Implement review feedback and add test for Tim's comment about a missing operator= in the primary template.

cor3ntin removed a subscriber: cor3ntin.Mon, Jul 5, 11:21 AM
cjdb added inline comments.Mon, Jul 5, 12:58 PM
libcxx/include/__ranges/copyable_box.h
41–42

Please move this to <concepts>: I plan to use this elsewhere! Also, it should be:

template<class _Tp>
concept destructible_object = destructible<_Tp> && is_object_v<_Tp>;

template<class _Tp>
concept move_constructible_object = move_constructible<_Tp> && destructible_object<_Tp>;

template<class _Tp>
concept copy_constructible_object = copy_constructible<_Tp> && move_constructible_object<_Tp>;

(If we need constructible_object_from, I'll add that later.)

48

Can this be [[no_unique_address]]?

ldionne marked 2 inline comments as done.Tue, Jul 6, 6:05 AM
ldionne added inline comments.
libcxx/include/__ranges/copyable_box.h
41–42

I suggest that we instead move it to <concepts> when we have that second use for it. As it is, it's really an implementation detail of copyable_box.

Also, http://eel.is/c++draft/range.copy.wrap#1.1 says:

copyable-box<T> constrains its type parameter T with copy_­constructible<T> && is_­object_­v<T>.

copy_­constructible<T> already includes destructible<T> transitively (via constructible_from, at least).

48

There's really no point in doing that AFAICT, because std::optional is never empty.

ldionne updated this revision to Diff 356696.Tue, Jul 6, 6:10 AM
ldionne marked 2 inline comments as done.

Add missing uses of _LIBCPP_HIDE_FROM_ABI. I despise that macro.

Quuxplusone added inline comments.Tue, Jul 6, 6:54 AM
libcxx/include/__ranges/copyable_box.h
41–42

+1 @ldionne. Also, there's certainly no need to introduce multiple subsuming concepts here. Subsumption is a tool for solving a problem (namely, concept overloading); we should use the tool if-and-only-if we are solving that problem (or of course if the paper standard is mandating us to do so).

48

[[no_unique_address]] also has a layout effect on non-empty types, if they're tail-padded: https://godbolt.org/z/qjMWo699x

However, AIUI, its effect never leaks out of a type altogether: it makes a difference only on a member of X that is directly followed by another member of X. So if you have a class with only one data member, like this version of __copyable_box, then adding [[no_unique_address]] to that unique data member will never make a difference. (I think.) https://godbolt.org/z/K3GPPaxbz

cjdb added inline comments.Tue, Jul 6, 8:45 AM
libcxx/include/__ranges/copyable_box.h
41–42

Sure, I'll ask Zoe to move it in D103056, or when single_view is implemented.

copy_­constructible<T> already includes destructible<T> transitively (via constructible_from, at least).

The point of my implementation is to create a subsumption hierarchy that doesn't eventually create a conflict between the groupings destructible<T> && is_object_v<T>, move_constructible<T> && is_object_v<T>, and copy_constructible<T> && is_object_v<T>. I don't want there to be a competing __move_constructible_object later on down the line, as I suspect there inevitably will be. Designing concepts is tricky business, and putting is_object_v here is arbitrary. It makes more sense to put it on the base concept and grow out, which is why destructible is at the base of all constructible concepts.

ldionne marked 3 inline comments as done.Tue, Jul 6, 11:43 AM
ldionne added inline comments.
libcxx/include/__ranges/copyable_box.h
41–42

If the Standard had used some exposition-only concept __copy_constructible_object to constrain F in transform_view, then I'd agree strongly. As it is, I'm lukewarm. Let's lift it once we have a couple uses for it.

48

That matches my understanding too. I don't think it's possible for the effect to leak out of the type since it must be possible to lay out objects of that type one after another in an array without messing up their alignment.

tcanens added inline comments.Tue, Jul 6, 11:54 AM
libcxx/include/__ranges/copyable_box.h
48
ldionne marked 3 inline comments as done.Tue, Jul 6, 12:48 PM
ldionne added inline comments.
libcxx/include/__ranges/copyable_box.h
48

Ugh, interesting. So it does "leak out" when the type is used as a member which is itself marked as [[no_unique_address]]. In that case, I guess it's worth adding it here to allow propagating the no-unique-addressness to the types that use copyable_box.

For a second, I was also concerned that it might behave like pragma pack(1) and mess up alignment, but it looks like the compiler does the only sane thing it can, and doesn't mess it up. Awesome.

ldionne updated this revision to Diff 356805.Tue, Jul 6, 12:49 PM
ldionne marked an inline comment as done.

Add [[no_unique_address]] on std::optional member as pointed out by @cjdb

Mordante accepted this revision.Tue, Jul 6, 12:51 PM

LGTM, one minor question.

libcxx/include/__ranges/copyable_box.h
119

Is there a reason to use [[no_unique_address]] here? Don't the same justifications hold true as stated in the primary template?

ldionne marked an inline comment as done.Tue, Jul 6, 12:55 PM
ldionne added inline comments.
libcxx/include/__ranges/copyable_box.h
119

Assuming you mean the justifications that I said above, then those were incorrect, so they don't apply.

But regardless, in this case no_unique_address's value here is clearer, since it means the class can occupy no space at all if _Tp is empty. And this will most likely be the case quite often, since I expect this class will end up holding stateless function objects (probably often through a lambda's closure type) quite often.

cjdb added inline comments.Tue, Jul 6, 1:20 PM
libcxx/include/__ranges/copyable_box.h
41–42

If the goalpost is going to be moved, let's get rid of it here too. The standard doesn't say anything about __copy_constructible_object at all, and if we're going to limit ourselves to only standard-provided exposition-only detail concepts: you're breaking ranks here.

In other words: this decision is arbitrary and inconsistent.

Quuxplusone added inline comments.Tue, Jul 6, 1:41 PM
libcxx/include/__ranges/copyable_box.h
41–42

Philosophically @cjdb you're right, let's eliminate the concept. But physically I don't think we can, because we need to both "constrain __copyable_box" and "provide a partial specialization of it," which means we need to repeat the primary template's constraint in two different places (currently lines 45 and 115), and for that we seem to need to use a concept, because if we simply repeated requires is_object_v<_Tp> in both places, that would be a different atomic constraint every place it lexically appeared. So we need to factor it out into a concept...

...except, right, why is __copyable_box constrained at all?! It's an implementation detail of libc++! It doesn't need to be constrained; the constraint will be applied way up at the transform_view or whatever. Constraining doesn't make sense at this level, because there's no "fallback"; there's no reason __copyable_box<Foo> needs to be SFINAE-friendly ill-formed, because we're never going to SFINAE on it. So you can actually just remove the constraint entirely. This fixes the complaint about the concept (no more concept), and also simplifies the code around that partial specialization.

Maybe add static_assert(std::is_object_v<_Tp>); inside the class definition, just for sanity-checking. We should never fail that assertion.

ldionne marked 3 inline comments as done.Tue, Jul 6, 2:14 PM

I'm not seeing any more blocking comments here. I'll ship this tomorrow morning unless someone raises a blocking comment.

We can lift __copy_constructible_object into a common header (<concepts>?) in the transform_view review or later as a NFC consolidation commit. I don't think it's worth blocking this review over that issue.

libcxx/include/__ranges/copyable_box.h
41–42

We're not limiting ourselves to exposition only concepts mentioned in the Standard as any kind of policy. I'm just saying it makes no sense to lift it into <concepts> before we even have a use for it somewhere else, since we *do* only have the exposition-only concepts mentioned in the Standard inside <concepts> so far.

If we do have a use for the concept elsewhere (transform_view seems to need it at the very least), and if it's conforming to use it there (I think it is), then I believe it would be reasonable to lift the concept into <concepts> and use it in transform_view (and wherever else makes sense).

This revision was not accepted when it landed; it landed in state Needs Review.Wed, Jul 7, 3:14 AM
This revision was automatically updated to reflect the committed changes.
ldionne marked an inline comment as done.