[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/drop_view.h:80
       if constexpr (_UseCache)
-        __cached_begin_.__set(__tmp);
+        __cached_begin_.__emplace(__tmp);
       return __tmp;
----------------
My 


================
Comment at: libcxx/include/__ranges/non_propagating_cache.h:45
+    // constructing the contained type from an iterator.
+    struct __wrapper {
+      template<class ..._Args>
----------------
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)


================
Comment at: libcxx/test/libcxx/ranges/range.nonprop.cache/emplace_deref.pass.cpp:75
+  // Make sure that we do not require the type to be movable when we emplace-deref it.
+  // Copy elision should be performed instead, see http://eel.is/c++draft/range.nonprop.cache#note-1.
+  {
----------------
Essentially that type will never be copied either so maybe also try an immovable type?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107932