[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>
----------------
ldionne wrote:
> 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?
It's too easy to get this wrong on the author's end, and too easy to miss on the reviewer's end. I want to make sure that it's very clear to all parties that a non-propagating cache is only necessary in situations when it is necessary. It may end up being that we often use `__non_propagating_cache<T, true>`, but that forces the author to think about the situation, and it makes sure that the reviewer sees the author has thought about it.

I think I should add this in light of the above:
```
inline constexpr bool __always_cache = true;
```


================
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;
----------------
ldionne wrote:
> 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.
> Is `non_propagating_cache` ever handed out to users? I don't think so, right?

No.

> 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.

How is it going to make it easier on us in the future? By inheriting from `std::optional`, we get exactly what we need at all times. There's zero maintenance here: we don't need to worry about the design, and we don't need to worry about API corner cases that `std::optional` has already taken into account.

I'd prefer inheriting correctness: that's why we're trying to share code among std/ranges algorithms, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102121