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

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

Quuxplusone added a subscriber: tcanens.
Quuxplusone added inline comments.


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


================
Comment at: libcxx/include/__ranges/transform_view.h:80
+  {
+    return __iterator<true>{*this, ranges::begin(__base_)};
+  }
----------------
Nit: These curly braces make me uneasy, just in general. What would go wrong if you changed all the curly braces throughout to parens? Is it worth doing? (I //hope// that because we control the overload set of `__iterator<true>`'s constructor, we can be sure that curly braces are safe here. But if we used parens from the get-go, we wouldn't //have// to be sure.)


================
Comment at: libcxx/include/__ranges/transform_view.h:258-262
+//   friend constexpr auto operator<=>(const __iterator& __x, const __iterator& __y)
+//     requires random_access_range<_Base> && three_way_comparable<iterator_t<_Base>>
+//   {
+//     return __x.__current_ <=> __y.__current_;
+//   }
----------------
What's the deal with this commented-out `operator<=>`?


================
Comment at: libcxx/include/__ranges/transform_view.h:73-74
+
+  constexpr __iterator<false> begin()
+    noexcept(noexcept(ranges::begin(__base_)))
+  {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103056