[libcxx-commits] [PATCH] D102135: [libcxx][ranges] adds _`copyable-box`_

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver accepted this revision.
zoecarver added a comment.

LGTM.



================
Comment at: libcxx/include/__ranges/copyable_box.h:111
+  class __copyable_box<_Tp> {
+    [[no_unique_address]] _Tp __val_;
+
----------------
ldionne wrote:
> tcanens wrote:
> > zoecarver wrote:
> > > 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. 
> > `no_unique_address` doesn't change the basic object model rule that two living objects of the same type must have distinct addresses.
> @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]]`).
> 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. 


================
Comment at: libcxx/include/__ranges/copyable_box.h:89
+
+    constexpr _Tp const& operator*() const { return *__val_; }
+    constexpr _Tp& operator*() { return *__val_; }
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > ldionne wrote:
> > > Quuxplusone wrote:
> > > > zoecarver wrote:
> > > > > This and the other's need to be noexcept. (After my patch, std::optional's star will also be noexcept.)
> > > > 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).
> > > @zoecarver  I'm not sure I understand why it needs to be `noexcept`. Can you please explain?
> > 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. 
> 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.
We decided in the review that we wanted `operator*` to have the correct noexceptness to implement `iter_move` and for QoI. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102135/new/

https://reviews.llvm.org/D102135