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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb 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>
----------------
zoecarver wrote:
> Could we constrain this further? 
Why should we constrain this further?


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


================
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> >);
----------------
zoecarver wrote:
> 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). 
Wholly, or just here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102121