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

[Zoe Carver via Phabricator via libcxx-commits]

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


================
Comment at: libcxx/include/__ranges/transform_view.h:18
+#include <__ranges/empty.h>
+#include <__ranges/copyable_box.h>
+#include <__ranges/view_interface.h>
----------------
Quuxplusone wrote:
> alphabetize plz
Held over from when it was named semiregular box.


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


================
Comment at: libcxx/include/__ranges/transform_view.h:98
+  constexpr __sentinel<true> end()
+    const noexcept(noexcept(ranges::end(__base_)))
+    requires range<const _View> &&
----------------
Quuxplusone wrote:
> This `const` is on the wrong line; ditto on line 79 above; and then please rearrange these getters into any reasonable order (e.g. `begin`, `begin const`, `end`, `end const`). Right now they're in the order `begin const, begin, end, end const`, which combined with the lack of trailing `const` keyword, confused the heck out of me.
Yes, it's confusing. Which is worse, how it is now, or having it be in a different order from the standard wording? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103056