Implements part of P2325.
Implements [range.copy.wrap].
Depends on D102119.
Differential D102135
[libcxx][ranges] adds _`copyable-box`_ ldionne on May 9 2021, 10:11 AM. Authored by
Details
Implements part of P2325. Depends on D102119.
Diff Detail
Unit Tests Event TimelineComment 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.
Comment Actions Need to update according to the recent changes made by P2325 (heads up: this will become __copyable_box).
Comment Actions responds to Zoe's feedback
Comment Actions This LGTM. Thanks.
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
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?
Comment Actions Yep, aka "transform_view has to work for captureful lambdas".
This should be two overloads so that it can be trivial when possible - which also happens to make the noexcept trivial to write .
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
Comment Actions This looks good to me sans the one small comment.
Comment Actions LGTM.
Comment Actions Implement review feedback and add test for Tim's comment about a missing operator= in the primary template.
Comment Actions LGTM, one minor question.
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.
|
Please update the module map.