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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 7 inline comments as done.
ldionne added inline comments.


================
Comment at: libcxx/include/__ranges/copyable_box.h:111
+  class __copyable_box<_Tp> {
+    [[no_unique_address]] _Tp __val_;
+
----------------
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]]`).


================
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.
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;
}
```


================
Comment at: libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/types.h:13
+#include <cassert>
+#include <__ranges/copyable_box.h>
+
----------------
Quuxplusone wrote:
> ldionne wrote:
> > Quuxplusone wrote:
> > > I'd rather see just `#include <ranges>` here.
> > 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.
> (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.)
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102135