[libcxx-commits] [PATCH] D103056: [libcxx][ranges] Add `ranges::transform_view`.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__ranges/transform_view.h:289
+  friend constexpr decltype(auto) iter_move(const __iterator& __i)
+    noexcept(noexcept(*__i))
+  {
----------------
As it stands, I believe the Spec has an issue. CC @tcanens . I think `iter_move(transform_view::iterator)` is never `noexcept`. It is defined in http://eel.is/c++draft/range.transform.iterator as:

```
constexpr decltype(auto) iter_move(const transform_view::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;
}
```

However, `*i.parent_->fun_` dereferences the `copyable-box` holding the function object, but `copyable-box` is specified to have the same API as `std::optional`. In the spec, `std::optional::operator*` is not `noexcept`, so the whole `noexcept(noexcept(invoke(*i.parent_->fun_, *i.current_)))` is always `noexcept(false)` unless the library provides a `noexcept` `std::optional::operator*` as an extension.

@tcanens Does that make sense? Is this LWG issue worthy?

(marking as Done since it's not an issue with this review per se).


================
Comment at: libcxx/include/__ranges/transform_view.h:60-61
+
+  _View __base_ = _View();
+  __copyable_box<_Fn> __func_;
+
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > zoecarver wrote:
> > > tcanens wrote:
> > > > Quuxplusone wrote:
> > > > > tcanens wrote:
> > > > > > Quuxplusone wrote:
> > > > > > > My intuition is that you want to switch these around:
> > > > > > > ```
> > > > > > > [[no_unique_address]] __copyable_box<_Fn> __func_;
> > > > > > > _View __base_ = _View();
> > > > > > > ```
> > > > > > > However, I remember @tcanens having counterintuitive-but-correct intuitions about field ordering in other reviews. It would be useful to write those down as guidelines, somewhere. I volunteer to blog it, if someone can explain it to me until I remember it. :)
> > > > > > After LWG3549 this doesn't really matter.
> > > > > I don't think LWG3549 presents an alternative to `view_base` for non-libc++ programmers, though, does it? "Civilian" (albeit Niebler-level) programmers are still going to be writing classes that derive from `view_base`, and so if we're making any layout decisions based on that derivation, our decisions should be unaffected by LWG3549.
> > > > > I'm thinking of concerns like this:
> > > > > https://godbolt.org/z/nr9sjbqzn
> > > > > ...Or is the idea that civilians should never use `view_base`, they should always use `view_interface<CRTP>` instead? (In that case, `view_base` probably should never have been specified in the standard?)
> > > > View adaptors that use `view_base` are saying "I don't care about the potential extra padding". I don't really care about making a wrapped external thing work better with some other external wrapper - it's trying to save other people from themselves.
> > > > 
> > > > `view_base` is still handy for concrete views (that aren't adaptors).
> > > I've made both of these `no_unique_address` and flipped the order. This will open us up to problems, though, if someone has an empty view and empty function where the function is copyable and has a copy ctor with side effects. If they use two transform views that are both marked as `no_unique_address` and try to assign one of them to the other, it won't actually invoke the copy ctor. I think Louis is addressing this in the copyable box patch, though. 
> > >  If they use two transform views that are both marked as no_unique_address and try to assign one of them to the other, it won't actually invoke the copy ctor.
> > 
> > I don't understand. Can you show a Godbolt that's as close as possible to what you're talking about? I would also accept a code snippet that displays the problem on libc++-after-this-patch, even if I had to compile it locally.
> > 
> > I wonder if you're talking about the fact that //`copyable-box`// defines its assignment operator in terms of a 1990s-flavored `if (this != &rhs)` test. You're right that that's going to do the wrong thing if we have two distinct //`copyable-box`// objects located at the same memory address. However, I think it's probably up to //`copyable-box`// to ensure somehow that that doesn't happen (and/or just a QoI issue).
> > I wonder if you're talking about the fact that copyable-box defines its assignment operator in terms of a 1990s-flavored if (this != &rhs) test. You're right that that's going to do the wrong thing if we have two distinct copyable-box objects located at the same memory address. However, I think it's probably up to copyable-box to ensure somehow that that doesn't happen (and/or just a QoI issue).
> 
> Yes, this is what I'm talking about, and yes I think this is copyable-box's problem (see my comment in that review). 
> 
> I think we're on the same page now, but if not, I can try to find an example. 
I concur, I think this is `copyable_box`'s problem. I'll address that over there, and add a test so you can see what we're talking about if you're still unsure.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/iterator/compare.pass.cpp:35
+  assert(iter1     >= iter2);
+  // TODO: operator<=> when three_way_comparable is implemented.
+
----------------
cjdb wrote:
> Can you add the tests and just  have them commented out for now please?
Or better, `#if !defined(_LIBCPP_VERSION)`, with a `TODO` comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103056