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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 20 inline comments as done.
zoecarver added inline comments.


================
Comment at: libcxx/include/__ranges/transform_view.h:73-74
+
+  constexpr __iterator<false> begin()
+    noexcept(noexcept(ranges::begin(__base_)))
+  {
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > Quuxplusone wrote:
> > > zoecarver wrote:
> > > > Quuxplusone wrote:
> > > > > Teeechnically... it's impossible for humans to write noexcept-clauses. ;) Could you test locally with an iterator type like this, and report back if it prints `OK!`?
> > > > > https://godbolt.org/z/fvxTo4n1v
> > > > Yep, it prints `OK!`. Want me to add a test like this? I think it should be covered by the noexcept tests, but maybe wouldn't hurt. 
> > > Aw, geez... Try again with this version, please. https://godbolt.org/z/v16bds7o1
> > > Did I mention it's impossible for humans to write noexcept-clauses? :D  (I accidentally left the `noexcept` off of my `Foo::begin` method.)
> > OK, it still prints `OK!` :P
> I forget the punch line now. I hypothesize that my original comment dated back to when this `begin()` had a noexcept-clause, which is now (fortunately) removed.
FWIW: this printed OK both with and without the noexcepts. 


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


================
Comment at: libcxx/include/__ranges/transform_view.h:126
+template<class, class>
+struct __iterator_category_base {};
+
----------------
Rename to __transform_view_iterator_category_base. 


================
Comment at: libcxx/include/__ranges/transform_view.h:167
+    : __current_(_VSTD::move(__current)), __parent_(_VSTD::addressof(__parent)) {}
+  constexpr __iterator(__iterator<false> __i)
+    requires _Const && convertible_to<iterator_t<_View>, iterator_t<_Base>>
----------------
CaseyCarter wrote:
> cjdb wrote:
> > I can't remember, but there might be a technical reason why the standard says `!Const` and not `false`. cc @tcanens and @CaseyCarter for fact checking.
> Per https://eel.is/c++draft/class.copy.ctor#5, this constructor declaration is ill-formed when `Const` is `false`.
Marking as complete. 


================
Comment at: libcxx/include/__ranges/transform_view.h:295
+
+  friend constexpr void iter_swap(const __iterator& __x, const __iterator& __y)
+    noexcept(noexcept(ranges::iter_swap(__x.__current_, __y.__current_)))
----------------
tcanens wrote:
> This has been removed by LWG3520.
Perfect :)


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/iterator/requirements.compile.pass.cpp:20-32
+template<class V, class F>
+concept IterMinusMinus = requires(std::ranges::iterator_t<std::ranges::transform_view<V, F>> iter) {
+  iter--;
+};
+static_assert(IterMinusMinus<BidirectionalView, IncrementConst>);
+static_assert(!IterMinusMinus<ForwardView, IncrementConst>);
+
----------------
cjdb wrote:
> Is this not just `static_assert(std::bidirectional_iterator<std::ranges::transform_view<BidirectionalView, IncrementConst>>);`? Similarly for below with `std::random_access_iterator`.
I updated based on your suggestion. I think it's kind of nice to spell out the exact requirements for each overload, but this is way clearer and simpler, so I think you're right, it is better. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103056