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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked an inline comment as done.
zoecarver 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
----------------
cjdb wrote:
> 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.
Sure, I'll move it.


================
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>>
----------------
cjdb wrote:
> 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.
Sorry, I thought @CaseyCarter was saying the opposite. 

Anyway, can you find an example where there's an observable difference between using false here and `!Const`? 

Here's an example that uses this ctor. I think the `requires` disabled the overload so the copy constructor is picked:
```
  std::ranges::transform_view<ContiguousView, Increment> transformView;
  auto iter = std::move(transformView).begin();
  std::ranges::iterator_t<std::ranges::transform_view<ContiguousView, Increment>> i2(iter);
  (void)i2;
```

I'll add the above snippet as a test for the iterator ctor. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103056