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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__ranges/non_propagating_cache.h:34
+namespace ranges {
+  template<class _Tp, bool __enable_cache>
+  requires is_object_v<_Tp>
----------------
I don't understand the purpose of this `__enable_cache` parameter. I believe this type should do what it does unconditionally, and when using it, one could write `conditional_t<SOME-CONDITION, __non_propagating_cache<T>, empty>` or something along those lines. WDYT?


================
Comment at: libcxx/include/__ranges/non_propagating_cache.h:36
+  requires is_object_v<_Tp>
+  struct __non_propagating_cache : optional<_Tp> {
+    using optional<_Tp>::optional;
----------------
cjdb wrote:
> Quuxplusone wrote:
> > It would be vastly more maintainable to make `__non_propagating_cache` have a //data member// of type `optional<_Tp>`, instead of using inheritance here. Please do so. (If you don't, I probably will at some point.)
> >  (If you don't, I probably will at some point.)
> 
> In doing so, you would become responsible for adding all `std::optional` member functions and free functions, adding tests to make sure they behave the same as the `std::optional` API, and you'll also be responsible for adding any new functionality that is added to `std::optional` in future standards (e.g. [[ http://wg21.link/p0798 | P0798 ]]). If that sounds more maintainable than the current implementation: sure, upload a patch for review after D102121 is merged into main.
Is `non_propagating_cache` ever handed out to users? I don't think so, right?

If that's correct, then I believe we don't have to actually implement the exact `std::optional` API, only what we need. And if that's the case, then I do agree that not inheriting from `std::optional` is going to make it easier to make changes in the future, so I'd hold it as a member instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102121