This is an archive of the discontinued LLVM Phabricator instance.

[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

cjdb created this revision.May 9 2021, 10:11 AM
cjdb requested review of this revision.May 9 2021, 10:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2021, 10:11 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante requested changes to this revision.Jun 6 2021, 2:37 AM

I didn't look closely at the unit test, but there seems several missing. "…semiregular-box<T> behaves exactly like optional<T> with the following differences…". The optional has several member functions not tested. For example constexpr T* operator->(), constexpr T& operator*(). I also miss the implementation of several of optional's constructors.

libcxx/include/CMakeLists.txt
41

Please update the module map.

libcxx/include/__ranges/semiregular_box.h
45

Remove std:: here and the line before.

52

other -> __other.

55

Please remove the braces when the if has a single statement.

This revision now requires changes to proceed.Jun 6 2021, 2:37 AM
cjdb planned changes to this revision.Jun 10 2021, 9:00 AM

Need to update according to the recent changes made by P2325 (heads up: this will become __copyable_box).

cjdb updated this revision to Diff 354019.Jun 23 2021, 10:33 AM
cjdb retitled this revision from [libcxx][ranges] adds _`semiregular-box`_ to [libcxx][ranges] adds _`copyable-box`_.
cjdb edited the summary of this revision. (Show Details)

updates to _`copyable-box`_

cjdb marked 3 inline comments as done.Jun 23 2021, 10:33 AM
cjdb added inline comments.
libcxx/include/__ranges/semiregular_box.h
55

Not done, I find it more readable to keep braces for all control structures.

zoecarver added inline comments.Jun 23 2021, 10:52 AM
libcxx/include/__ranges/copyable_box.h
31 ↗(On Diff #354019)

Do we need both of these conditions? I thought that the latter subsumed the former.

52 ↗(On Diff #354019)

Is this missing the "If copyable<T> is not modeled..."?

68 ↗(On Diff #354019)

Same as above.

libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/base.compile.pass.cpp
15 ↗(On Diff #354019)

Hmm, I know this is a libc++ only test, but I still think maybe it should include ranges. (Same elsewhere.)

libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/default_initializable.pass.cpp
67 ↗(On Diff #354019)

What about a type that had a noexcept(false) default ctor?

libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/properties.compile.pass.cpp
20 ↗(On Diff #354019)

Do you really need the base test if you have this?

30 ↗(On Diff #354019)

Can you rename this to move_only or something?

cjdb updated this revision to Diff 354028.Jun 23 2021, 11:09 AM
cjdb marked 3 inline comments as done.

responds to Zoe's feedback

libcxx/include/__ranges/copyable_box.h
52 ↗(On Diff #354019)

See the defaulted set of constructors.

68 ↗(On Diff #354019)

See the defaulted set of constructors.

libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/base.compile.pass.cpp
15 ↗(On Diff #354019)

<ranges> doesn't (and shouldn't) include this header, so we need to explicitly include it.

libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/properties.compile.pass.cpp
20 ↗(On Diff #354019)

Thanks, I forgot I put this test here. Deleted base.compile.pass.cpp.

30 ↗(On Diff #354019)

The current name spells out exactly what's missing. move_only implies copy-assignment is false.

zoecarver accepted this revision as: zoecarver.Jun 23 2021, 11:20 AM

This LGTM. Thanks.

libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/base.compile.pass.cpp
15 ↗(On Diff #354019)

Hmm, maybe we should rename it, then. Or put it elsewhere... WDYT?

libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/properties.compile.pass.cpp
30 ↗(On Diff #354019)

Sorry I misread the name on the first go around. This is good with me.

Optional: would it make sense to update drop_view in this patch to make sure there are no issues when it's actually getting used somewhere? Or maybe as a second patch that passes CI before this one lands?

cjdb added a comment.Jun 23 2021, 11:37 AM

Optional: would it make sense to update drop_view in this patch to make sure there are no issues when it's actually getting used somewhere? Or maybe as a second patch that passes CI before this one lands?

drop_view has no relationship with this patch.

drop_view has no relationship with this patch.

You're right. I got this and non-propagating-cache confused because I'm updating transform_view right now, and that does use this patch. So, never mind :)

ldionne requested changes to this revision.Jun 23 2021, 1:35 PM

At first, I didn't understand the purpose of this type. Now I think I do - it's to turn something that is copy-constructible (but maybe not copy assignable) into something that is both copy-constructible and copy-assignable. It does that by using an optional and basically doing destroy-then-copy-construct in the assignment operator.

However, we are missing the optimization mentioned at http://eel.is/c++draft/range.adaptors#range.copy.wrap-2. Basically, if we know that T is copyable or is_nothrow_move_constructible && is_nothrow_copy_constructible, we can forego the bool engaged bit of information and store a straight T. Indeed, if the type is copyable, then the operator= is trivial to implement, by using the operator= provided by T itself. Similarly, if the type is nothrow_copy_constructible, we know we can do the destroy-and-then-construct dance without the copy construction throwing an exception, and hence we're able to provide the correct exception guarantee without introducing an empty state.

We can implement it roughly as:

template<class _Tp>
  requires copyable<_Tp> || (is_nothrow_move_constructible_v<_Tp> && is_nothrow_copy_constructible_v<_Tp>)
struct __copyable_box<_Tp> {
  __copyable_box& operator=(__copyable_box const& __other) noexcept(to-be-figured-out) {
    if constexpr (copyable<_Tp>) {
      __val_ = __other.__val_;
    } else { // is_nothrow_xxx is satisfied
      _VSTD::destroy_at(&__val_);
      _VSTD::construct_at(&__val_, other.__val_);
    }
    return *this;
  }

  // The rest follows naturally
private:
  [[no_unique_address]] _Tp __val_; // no-unique-address not mandatory, but why not!
};

WDYT?

libcxx/include/__ranges/copyable_box.h
13 ↗(On Diff #354028)

I don't understand this comment, can you explain? In particular, what is a customization variable?

36–37 ↗(On Diff #354028)

Unless there is a reason to give special treatment to copy_constructible over is_object_v, I would write it as:

template <class _Tp>
requires copy_constructible<_Tp> && is_object_v<_Tp>

WDYT?

38 ↗(On Diff #354028)

Please make this a member. Let's expose the smallest API that it makes sense to expose, it eases maintenance and understanding of how the API is used.

When I read http://eel.is/c++draft/range.adaptors#range.copy.wrap-1:

copyable-box<T> behaves exactly like optional<T> with the following differences

I don't read this as meaning "it has the same API as optional<T>". I understand it as being an internal type with whatever API is needed to fulfill the implementation, but with a behavior equivalent to optional<T> for that API (except where specified otherwise). MSVC also doesn't seem to think that the whole optional API is needed: https://github.com/microsoft/STL/blob/master/stl/inc/ranges#L247.

Also, not all standard library types are written with the use case of being inherited from in mind. It does work, sure, but it may end up being somewhat weird. For example, when you say

using optional<_Tp>::optional;

below, you end up bringing in all the constructors like this one:

template < class U >
optional( const optional<U>& other );

It certainly feels weird to me that the following line would compile:

__copyable_box<T> box = std::make_optional(t);
52 ↗(On Diff #354028)

Maybe we should add requires !copyable<_Tp> here, and requires !movable<_Tp> below to make it clearer?

This revision now requires changes to proceed.Jun 23 2021, 1:35 PM

At first, I didn't understand the purpose of this type. Now I think I do - it's to turn something that is copy-constructible (but maybe not copy assignable) into something that is both copy-constructible and copy-assignable. It does that by using an optional and basically doing destroy-then-copy-construct in the assignment operator.

Yep, aka "transform_view has to work for captureful lambdas".

__copyable_box& operator=(__copyable_box const& __other) noexcept(to-be-figured-out) {
  if constexpr (copyable<_Tp>) {
    __val_ = __other.__val_;
  } else { // is_nothrow_xxx is satisfied
    _VSTD::destroy_at(&__val_);
    _VSTD::construct_at(&__val_, other.__val_);
  }
  return *this;
}

This should be two overloads so that it can be trivial when possible - which also happens to make the noexcept trivial to write .

ldionne commandeered this revision.Jun 24 2021, 10:24 AM
ldionne edited reviewers, added: cjdb; removed: ldionne.

Commandeering per discussion w/ Chris.

cjdb added inline comments.Jun 24 2021, 10:31 AM
libcxx/include/__ranges/copyable_box.h
36–37 ↗(On Diff #354028)

That's not idiomatic use of concepts.

52 ↗(On Diff #354028)

That partially defeats the point of concepts, which is to not need doing that.

libcxx/include/__ranges/copyable_box.h
13 ↗(On Diff #354028)

That's accidentally copy-pasted from enable_borrowed_range.h. These comment lines can just vanish.

52 ↗(On Diff #354028)

I put my thoughts on this question here:
https://quuxplusone.github.io/blog/2021/06/07/tag-dispatch-and-concept-overloading/#it-seems-to-be-the-current-wisdo
If the overloads were complicated or separated by dozens of lines of code, I'd think Louis' point is reasonable. As it is, I'm neutral.

libcxx/include/module.modulemap
560 ↗(On Diff #354028)

Incidentally, another side benefit of the planned rewrite to compose-not-inherit from optional is that whatever this little export optional hack is, will no longer be necessary.

ldionne marked 7 inline comments as done.Jun 29 2021, 7:24 AM
ldionne added inline comments.
libcxx/include/__ranges/copyable_box.h
36–37 ↗(On Diff #354028)

I'd be quite curious to hear what's the rationale here.

But regardless, this is going to change and become constrained by a helper concept (which helps with the partial specialization for the copy-assignable optimization), so this will become moot when I update the patch.

52 ↗(On Diff #354028)

That partially defeats the point of concepts, which is to not need doing that.

Hmm, I guess I agree. This won't be needed anyways once the specializations are split out.

libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/properties.compile.pass.cpp
30 ↗(On Diff #354019)

I need to change the tests quite a bit because the type changed so much, so I don't think this will be relevant anymore when I update.

ldionne marked 2 inline comments as done.Jun 29 2021, 2:04 PM
ldionne updated this revision to Diff 355358.Jun 29 2021, 2:06 PM

Overhaul __copyable_box and its tests.

I do expect I'll have to tweak the tests for some testing configurations such as -fno-exceptions and
perhaps a few other things, however this should be ready for review.

ldionne added inline comments.Jun 29 2021, 2:09 PM
libcxx/include/__ranges/copyable_box.h
56 ↗(On Diff #355358)

Oops, I just realized I'm missing a test for this constructor.

CaseyCarter added inline comments.Jun 29 2021, 3:23 PM
libcxx/include/__ranges/copyable_box.h
124 ↗(On Diff #355358)

Why not requires movable<_Tp>? (Don't forget to update the redundant constraint on line 135.) This enables you to relax the requirements on this specialization to copyable<_Tp> || (is_nothrow_copy_constructible_v<_Tp> && (movable<_Tp> || is_nothrow_move_constructible_v<_Tp>)). Not that I think there's much to be gained thereby - it's hard to envision a nothrow-copy_constructible type that isn't also nothrow-move_constructible.

This looks good to me sans the one small comment.

libcxx/include/__ranges/copyable_box.h
51 ↗(On Diff #355358)

std::

51 ↗(On Diff #355358)

Just curious: why is having a member better than inheriting?

libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/assign.move.pass.cpp
112 ↗(On Diff #355358)

You're going to need an #if exceptions-enabled here and in similar places in other tests.

libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/properties.compile.pass.cpp
37 ↗(On Diff #355358)

I might argue that we don't really need this line. But I suppose it doesn't really hurt.

(The reason being, we don't really care if these are equal in size, we care if the throwing behavior works.)

zoecarver added inline comments.Jun 29 2021, 3:38 PM
libcxx/include/__ranges/copyable_box.h
51 ↗(On Diff #355358)

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
51 ↗(On Diff #355358)
56 ↗(On Diff #355358)

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.

63 ↗(On Diff #355358)

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.

91 ↗(On Diff #355358)

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

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

113–114 ↗(On Diff #355358)

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

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

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

41 ↗(On Diff #355358)

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

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

libcxx/include/__ranges/copyable_box.h
89 ↗(On Diff #355358)

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.Jul 1 2021, 9:28 AM
ldionne added inline comments.
libcxx/include/__ranges/copyable_box.h
89 ↗(On Diff #355358)

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

124 ↗(On Diff #355358)

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

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

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

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.Jul 1 2021, 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.Jul 1 2021, 10:02 AM
libcxx/include/__ranges/copyable_box.h
111 ↗(On Diff #355911)

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.

89 ↗(On Diff #355358)

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.

libcxx/include/__ranges/copyable_box.h
89 ↗(On Diff #355358)

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.

libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/types.h
13 ↗(On Diff #355358)

(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.Jul 1 2021, 12:41 PM
libcxx/include/__ranges/copyable_box.h
111 ↗(On Diff #355911)

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.Jul 1 2021, 12:59 PM
ldionne added inline comments.
libcxx/include/__ranges/copyable_box.h
111 ↗(On Diff #355911)

@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]]).

89 ↗(On Diff #355358)

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;
}
libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/types.h
13 ↗(On Diff #355358)

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.Jul 1 2021, 12:59 PM
ldionne marked 3 inline comments as done.

Update per review comments.

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

LGTM.

libcxx/include/__ranges/copyable_box.h
111 ↗(On Diff #355911)

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.

89 ↗(On Diff #355358)

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

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

tcanens added inline comments.Jul 1 2021, 9:08 PM
libcxx/include/__ranges/copyable_box.h
75 ↗(On Diff #355992)

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;
130–131 ↗(On Diff #355992)

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

tcanens added inline comments.Jul 1 2021, 9:17 PM
libcxx/include/__ranges/copyable_box.h
137–138 ↗(On Diff #355992)
146–147 ↗(On Diff #355992)
Quuxplusone added inline comments.Jul 2 2021, 8:14 AM
libcxx/include/__ranges/copyable_box.h
130–131 ↗(On Diff #355992)

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.Jul 2 2021, 8:18 AM
libcxx/include/__ranges/copyable_box.h
89 ↗(On Diff #355358)

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.Jul 2 2021, 8:19 AM
libcxx/include/__ranges/copyable_box.h
130–131 ↗(On Diff #355992)

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.Jul 2 2021, 8:29 AM
libcxx/include/__ranges/copyable_box.h
130–131 ↗(On Diff #355992)

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.Jul 2 2021, 12:43 PM
libcxx/include/__ranges/copyable_box.h
89 ↗(On Diff #355358)

3520 didn't remove iter_move.

ldionne marked 11 inline comments as done.Jul 5 2021, 10:06 AM
ldionne added inline comments.
libcxx/include/__ranges/copyable_box.h
130–131 ↗(On Diff #355992)

TIL!

ldionne updated this revision to Diff 356523.Jul 5 2021, 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.Jul 5 2021, 11:21 AM
cjdb added inline comments.Jul 5 2021, 12:58 PM
libcxx/include/__ranges/copyable_box.h
40–41 ↗(On Diff #356523)

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.)

47 ↗(On Diff #356523)

Can this be [[no_unique_address]]?

ldionne marked 2 inline comments as done.Jul 6 2021, 6:05 AM
ldionne added inline comments.
libcxx/include/__ranges/copyable_box.h
40–41 ↗(On Diff #356523)

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).

47 ↗(On Diff #356523)

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

ldionne updated this revision to Diff 356696.Jul 6 2021, 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.Jul 6 2021, 6:54 AM
libcxx/include/__ranges/copyable_box.h
40–41 ↗(On Diff #356523)

+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).

47 ↗(On Diff #356523)

[[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.Jul 6 2021, 8:45 AM
libcxx/include/__ranges/copyable_box.h
40–41 ↗(On Diff #356523)

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.Jul 6 2021, 11:43 AM
ldionne added inline comments.
libcxx/include/__ranges/copyable_box.h
40–41 ↗(On Diff #356523)

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.

47 ↗(On Diff #356523)

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.Jul 6 2021, 11:54 AM
libcxx/include/__ranges/copyable_box.h
47 ↗(On Diff #356523)
ldionne marked 3 inline comments as done.Jul 6 2021, 12:48 PM
ldionne added inline comments.
libcxx/include/__ranges/copyable_box.h
47 ↗(On Diff #356523)

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.Jul 6 2021, 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.Jul 6 2021, 12:51 PM

LGTM, one minor question.

libcxx/include/__ranges/copyable_box.h
118 ↗(On Diff #356696)

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.Jul 6 2021, 12:55 PM
ldionne added inline comments.
libcxx/include/__ranges/copyable_box.h
118 ↗(On Diff #356696)

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.Jul 6 2021, 1:20 PM
libcxx/include/__ranges/copyable_box.h
40–41 ↗(On Diff #356523)

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.Jul 6 2021, 1:41 PM
libcxx/include/__ranges/copyable_box.h
40–41 ↗(On Diff #356523)

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.Jul 6 2021, 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
40–41 ↗(On Diff #356523)

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.Jul 7 2021, 3:14 AM
This revision was automatically updated to reflect the committed changes.
ldionne marked an inline comment as done.