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

[Michael Schellenberger Costa via Phabricator via libcxx-commits]

miscco 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>
----------------
Quuxplusone wrote:
> 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/).")
You are missing the new and shiny `split_view` In begin we call `find-next` `http://eel.is/c++draft/range.split.view#3 with the explicit remark that we need to cache it http://eel.is/c++draft/range.split.view#4

`find-next` returns a subrange<It>



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107932