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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb requested changes to this revision.
cjdb added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__ranges/concepts.h:108
+  template<bool _Const, class _Tp>
+  using __maybe_const = conditional_t<_Const, const _Tp, _Tp>;
 } // namespace ranges
----------------
Quuxplusone wrote:
> 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.
This isn't a range concept (and it's not specific to ranges even if that's the only current client), so I'd put it in `<type_traits>`. Non-blocking.


================
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>>
----------------
zoecarver wrote:
> 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. 
Why? It's still ill-formed and needs to be changed to `!_Const` AIUI.


================
Comment at: libcxx/test/std/utilities/optional/optional.object/optional.object.observe/dereference.pass.cpp:47
         ASSERT_SAME_TYPE(decltype(*opt), X&);
+        LIBCPP_STATIC_ASSERT(noexcept(*opt));
         // ASSERT_NOT_NOEXCEPT(*opt);
----------------
zoecarver wrote:
> cjdb wrote:
> > Can you please move the `optional` changes to their own commit? I'd prefer to keep this one solely focussed on `transform_view`.
> Done. 
I still see `optional` diffs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103056