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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__ranges/transform_view.h:60-61
+
+  _View __base_ = _View();
+  __copyable_box<_Fn> __func_;
+
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103056