[libcxx-commits] [PATCH] D102121: [libcxx][ranges] adds _`non-propagating-cache`_

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/include/__ranges/non_propagating_cache.h:34
+namespace ranges {
+  template<class _Tp, bool __enable_cache>
+  requires is_object_v<_Tp>
----------------
Could we constrain this further? 


================
Comment at: libcxx/include/__ranges/non_propagating_cache.h:35
+  template<class _Tp, bool __enable_cache>
+  requires is_object_v<_Tp>
+  struct __non_propagating_cache : optional<_Tp> {
----------------
Indent. 


================
Comment at: libcxx/include/__ranges/non_propagating_cache.h:38
+    using optional<_Tp>::optional;
+    inline static constexpr bool cache_enabled = true;
+
----------------
This name should be mangled.


================
Comment at: libcxx/test/libcxx/ranges/range.nonprop.cache/cache_disabled.compile.pass.cpp:20
+
+static_assert(std::is_empty_v<std::ranges::__non_propagating_cache<std::array<int, 10>, false> >);
+static_assert(std::is_empty_v<std::ranges::__non_propagating_cache<std::array<int, 10>::iterator, false> >);
----------------
This isn't so much a request for a changes as a question: most of the time do we want to store a whole range or just an iterator. At least for drop_view, I feel like we'd only need an iterator.

In light of that, maybe test this with more than one type (it doesn't have to be a range or an iterator, in fact). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102121