[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:36
+  requires is_object_v<_Tp>
+  struct __non_propagating_cache : optional<_Tp> {
+    using optional<_Tp>::optional;
----------------
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.


================
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> {
----------------
zoecarver wrote:
> Indent. 
@ldionne [[ https://reviews.llvm.org/D99691#inline-938990 | left it up to me ]] to decide how libc++ formats requires clauses, and [[ https://reviews.llvm.org/D99691#inline-939014 | I decided that they shouldn't be indented ]].


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102121