Implements part of P2325.
Implements [range.copy.wrap].
Depends on D102119.
Details
Details
- Reviewers
EricWF zoecarver Mordante curdeius • Quuxplusone cjdb - Group Reviewers
Restricted Project - Commits
- rG6829db727e9e: [libc++] Implement copyable-box from Ranges
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
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. | |
Comment Actions
Need to update according to the recent changes made by P2325 (heads up: this will become __copyable_box).
| libcxx/include/__ranges/semiregular_box.h | ||
|---|---|---|
| 55 | Not done, I find it more readable to keep braces for all control structures. | |
| 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? |
Comment Actions
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. |
Comment Actions
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. |
Comment Actions
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?
Comment Actions
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 :)
Comment Actions
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:
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? |
Comment Actions
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 .
| 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: |
| 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. |
| 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) |
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. |
Comment Actions
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.
| libcxx/include/__ranges/copyable_box.h | ||
|---|---|---|
| 56 ↗ | (On Diff #355358) | Oops, I just realized I'm missing a test for this constructor. |
| 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. |
Comment Actions
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.) |
| 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. |
| 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. |
| 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. |
| 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. |
| 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? 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.) |
| 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. |
| 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) |
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. |
Comment Actions
LGTM.
| libcxx/include/__ranges/copyable_box.h | ||
|---|---|---|
| 111 ↗ | (On Diff #355911) |
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. |
| 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. |
| 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.) So this code is technically correct as written. But stylistically I agree: writing constexpr here is redundant and should be removed. |
| libcxx/include/__ranges/copyable_box.h | ||
|---|---|---|
| 130–131 ↗ | (On Diff #355992) |
That's not what https://timsong-cpp.github.io/cppwp/dcl.fct.def.default#3.sentence-1 says. |
| 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 |
| libcxx/include/__ranges/copyable_box.h | ||
|---|---|---|
| 89 ↗ | (On Diff #355358) | 3520 didn't remove iter_move. |
| libcxx/include/__ranges/copyable_box.h | ||
|---|---|---|
| 130–131 ↗ | (On Diff #355992) | TIL! |
Comment Actions
Implement review feedback and add test for Tim's comment about a missing operator= in the primary template.
| 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]]? |
| 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:
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. |
| 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 |
| 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.
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. |
| 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. |
| libcxx/include/__ranges/copyable_box.h | ||
|---|---|---|
| 47 ↗ | (On Diff #356523) | Not quite: https://godbolt.org/z/Yhs6MqzYn |
| 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. |
Comment Actions
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? |
| 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. |
| 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. |
| 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. |
Comment Actions
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). |
Please update the module map.