[libcxx-commits] [PATCH] D107932: [libc++] Do not require movability in __non_propagating_cache::__emplace_deref

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__ranges/non_propagating_cache.h:45
+    // constructing the contained type from an iterator.
+    struct __wrapper {
+      template<class ..._Args>
----------------
miscco wrote:
> I would not go that route. 
> 
> There are more places where you would want to reuse `__non_propagating_cache`, e.g. with `views::split` you want to cache the call to `search(__view.begin(), __pattern)`. That would not be possible to achieve when hard coding the wrapper in `__non_propagating_cache`
> 
> It is much easier to pull that wrapper as an implementation detail into `views::join` and let `__non_propagating_cache` be a generic utility
> 
> See (https://github.com/microsoft/STL/blob/a9321cfe53ea31a7e197d5d8336167d6ca3de8b6/stl/inc/ranges#L2962-L2975)
I see a //`non-propagating-cache`// in `lazy_split_view`
http://eel.is/c++draft/range.adaptors#range.lazy.split.outer-1
but it's initialized only with the result of `ranges::find`, which is an iterator, right? I don't see anywhere where we'd need to cache the result of `ranges::search`.
I don't see a //`non-propagating-cache`// actually specified for `split_view`, although the English wording does talk about "caches the result [of `begin()`]" in http://eel.is/c++draft/range.adaptors#range.split.view-4

However, if we anticipate needing to emplace more things than just `*it`, then it would be reasonable IMO to augment or replace this `__emplace_deref(It)` with a more general `__emplace_from(F)` that takes an arbitrary prvalue-producing lambda. (What I've called the "[super constructing super elider](https://quuxplusone.github.io/blog/2018/03/29/the-superconstructing-super-elider/).")


================
Comment at: libcxx/include/__ranges/non_propagating_cache.h:93-97
+    template<class _Iter>
     _LIBCPP_HIDE_FROM_ABI
-    constexpr void __set(_Tp const& __value) { __value_.emplace(__value); }
+    constexpr _Tp& __emplace_deref(_Iter const& __iter) {
+      return __value_.emplace(__deref_tag{}, __iter).__t_;
+    }
----------------
FYI: With a super-eliding `__emplace_from`, this would turn into
```
  template<class _Fn>
  _LIBCPP_HIDE_FROM_ABI
  constexpr _Tp& __emplace_from(_Fn __iter) {
    return __value_.emplace(__from_tag{}, _Fn).__t_;
  }

  template<class _Iter>
  _LIBCPP_HIDE_FROM_ABI
  constexpr _Tp& __emplace_deref(_Iter const& __iter) {
    return __emplace_from([&]() -> decltype(auto) { return *__iter; });
  }
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107932