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

[Tim Song via Phabricator via libcxx-commits]

tcanens added inline comments.


================
Comment at: libcxx/include/__ranges/copyable_box.h:89
+
+    constexpr _Tp const& operator*() const { return *__val_; }
+    constexpr _Tp& operator*() { return *__val_; }
----------------
Quuxplusone wrote:
> ldionne wrote:
> > zoecarver wrote:
> > > 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. 
> > 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;
> > }
> > ```
> For the record, the offending `iter_move` has been eliminated by [LWG3520](https://wg21.link/lwg3520).
> However, I don't object to marking `copyable-box::operator*` as `noexcept`, just for QoI.
3520 didn't remove `iter_move`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102135