[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/concepts.h:108
+  template<bool _Const, class _Tp>
+  using __maybe_const = conditional_t<_Const, const _Tp, _Tp>;
 } // namespace ranges
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > zoecarver wrote:
> > > ldionne wrote:
> > > > Is this used anywhere else other than in `transform_view`? If not, let's put it in `transform_view.h` only.
> > > Yes, it's used in many views. 
> > FWIW, I think the libc++ style here would be to name it either `__maybe_const_t` or `_MaybeConst`, and then stick it in `<type_traits>`.
> > 
> > (The otherwise-badly-named `_Parent` and `_Base` typedefs repeated below are merely mirroring the Standard exposition-only wording; see `Parent` and `Base` in https://en.cppreference.com/w/cpp/ranges/transform_view/sentinel . So even though they're awfully ugly, I think `_Parent` and `_Base` are probably following the right course of action.)
> If you'd rather me put it in type traits that fine with me. But I want to keep the name the same. All of these exposition only concepts and type traits we do a mechanical transformation of `concept-or-trait` -> `__concept_or_trait`. Makes it easy to keep track of things, verify correctness, and reduce duplication. 
Ah, I see https://eel.is/c++draft/ranges.syn has the exposition-only //`maybe-const`//. Weird that cppreference avoids //`maybe-const`// and just does the English description e.g. "`const V` if `Const` is true, otherwise `V`." Anyway, LGTM.


================
Comment at: libcxx/include/__ranges/transform_view.h:18
+#include <__ranges/empty.h>
+#include <__ranges/copyable_box.h>
+#include <__ranges/view_interface.h>
----------------
alphabetize plz


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


================
Comment at: libcxx/include/__ranges/transform_view.h:98
+  constexpr __sentinel<true> end()
+    const noexcept(noexcept(ranges::end(__base_)))
+    requires range<const _View> &&
----------------
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.


================
Comment at: libcxx/include/__ranges/transform_view.h:338
+    requires sentinel_for<sentinel_t<_Base>, iterator_t<__maybe_const<_OtherConst, _View>>>
+  friend constexpr bool operator==(const __iterator<_OtherConst>& __x, const __sentinel __y) {
+    return __x.__current_ == __y.__end_;
----------------
Either a missing `&` in `const __sentinel`, or a superfluous `const` — please decide which.
(On line 345 you went with the missing-`&` interpretation.)
[Grep your codebase today!](https://quuxplusone.github.io/blog/2019/01/03/const-is-a-contract/#grep-your-codebase-today)


================
Comment at: libcxx/include/optional:936
     const value_type&&
-    operator*() const&&
+    operator*() const&& noexcept
     {
----------------
These noexcepts look great to me, but perhaps we should add a simple ~five-line test for them in `libcxx/test/libcxx/`.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/general.pass.cpp:39
+auto withRandom(R range, Fn func = Fn()) {
+  return std::ranges::transform_view(range, std::bind_front(func, std::rand()));
+}
----------------
I don't understand what's going on here with `std::bind_front`, nor the defaulted function argument `func = Fn()`, but surely our unit tests should not depend on the behavior of `rand()`.
Please switch to e.g. `[](int x) { return x + 1; }` and remove the `bind` and default-function-argument stuff.



================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/general.pass.cpp:45
+  return std::ranges::transform_view(a, [&a, &b, join](auto& x) {
+    auto idx = (&x) - a;
+    return join(x, b[idx]);
----------------
Is the point of this test to verify that `x` is in fact a reference into the original viewed range, and not a copy? If so, this could be more explicit. If not, then again this seems overly confusing and complicated.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/general.pass.cpp:50-54
+template<>
+constexpr bool std::ranges::enable_view<std::vector<int>> = true;
+
+template<>
+constexpr bool std::ranges::enable_view<std::string_view> = true;
----------------
I assume lines 50-54 were here for polyfill purposes; they can be removed now that `enable_view` is in master, right?


================
Comment at: libcxx/test/support/test_iterators.h:234
 
-    TEST_CONSTEXPR_CXX14 reference operator*() const {return *it_;}
+    TEST_CONSTEXPR_CXX14 reference operator*() const noexcept {return *it_;}
     TEST_CONSTEXPR_CXX14 pointer operator->() const {return it_;}
----------------
This shouldn't be necessary.  Not necessarily a //bad// idea, but unnecessary.

If we're going to `noexcept`-qualify our test iterators' `operator*`, `operator->`, and `operator[]`, that should be done consistently across //all// of the test iterator types, and it should be done in a separate PR so we can see what effect it has (if any) on the existing test suite.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103056