[libcxx-commits] [PATCH] D107932: [libc++] Do not require movability in __non_propagating_cache::__emplace_deref

Mon Aug 16 09:10:26 PDT 2021

ldionne added inline comments.

================
Comment at: libcxx/include/__ranges/drop_view.h:80
       if constexpr (_UseCache)
-        __cached_begin_.__set(__tmp);
+        __cached_begin_.__emplace(__tmp);
       return __tmp;
----------------
miscco wrote:
> My 
Did you leave this comment by mistake?

================
Comment at: libcxx/include/__ranges/non_propagating_cache.h:45
+    // constructing the contained type from an iterator.
+    struct __wrapper {
+      template<class ..._Args>
----------------
miscco wrote:
> Quuxplusone wrote:
> > miscco wrote:
> > > I would not go that route. 
> > > 
> > > There are more places where you would want to reuse `__non_propagating_cache`, e.g. with `views::split` you want to cache the call to `search(__view.begin(), __pattern)`. That would not be possible to achieve when hard coding the wrapper in `__non_propagating_cache`
> > > 
> > > It is much easier to pull that wrapper as an implementation detail into `views::join` and let `__non_propagating_cache` be a generic utility
> > > 
> > > See (https://github.com/microsoft/STL/blob/a9321cfe53ea31a7e197d5d8336167d6ca3de8b6/stl/inc/ranges#L2962-L2975)
> > I see a //`non-propagating-cache`// in `lazy_split_view`
> > http://eel.is/c++draft/range.adaptors#range.lazy.split.outer-1
> > but it's initialized only with the result of `ranges::find`, which is an iterator, right? I don't see anywhere where we'd need to cache the result of `ranges::search`.
> > I don't see a //`non-propagating-cache`// actually specified for `split_view`, although the English wording does talk about "caches the result [of `begin()`]" in http://eel.is/c++draft/range.adaptors#range.split.view-4
> > 
> > However, if we anticipate needing to emplace more things than just `*it`, then it would be reasonable IMO to augment or replace this `__emplace_deref(It)` with a more general `__emplace_from(F)` that takes an arbitrary prvalue-producing lambda. (What I've called the "[super constructing super elider](https://quuxplusone.github.io/blog/2018/03/29/the-superconstructing-super-elider/).")
> You are missing the new and shiny `split_view` In begin we call `find-next` `http://eel.is/c++draft/range.split.view#3 with the explicit remark that we need to cache it http://eel.is/c++draft/range.split.view#4
> 
> `find-next` returns a subrange<It>
> 
Hmm, I quite like Arthur's idea with a general `__emplace_from`. It will allow us to easily elide from arbitrary expressions.

================
Comment at: libcxx/include/__ranges/non_propagating_cache.h:85-88
     _LIBCPP_HIDE_FROM_ABI
-    constexpr _Tp& operator*() { return *__value_; }
+    constexpr _Tp& operator*() { return __value_->__t_; }
     _LIBCPP_HIDE_FROM_ABI
+    constexpr _Tp const& operator*() const { return __value_->__t_; }
----------------
Quuxplusone wrote:
> Any appetite for making these named members (e.g. `npc.__value()`)?
> Besides my usual advice that operators can be harder to read and/or go-to-definition of, it also just occurred to me that operators are subject to ADL but member functions are not:
> https://godbolt.org/z/EsasGzoad
> (I don't think the Standard cares about ADL in this instance; but any time I see a chance to ADL-proof something... ;))
> Any appetite for making these named members (e.g. `npc.__value()`)?

Not really, I think `operator*` is pretty idiomatic and I see no strong enough reason to change.

================
Comment at: libcxx/test/libcxx/ranges/range.nonprop.cache/emplace.pass.cpp:25
+  template<int J>
+  constexpr friend bool operator==(X const& a, X<J> const& b) { return I == J && a.value == b.value; }
+};
----------------
Quuxplusone wrote:
> `friend constexpr` https://quuxplusone.github.io/blog/2021/04/03/static-constexpr-whittling-knife/
> and consider line-breaking somewhere.
> 
> High-level comment: I'm not sure what benefit you get from the templated `operator==` or the use of `tuple`. If you just want something with a couple of weird constructors, why not `string`?
> ```
> using Cache = std::ranges::__non_propagating_cache<std::string>;
> Cache cache;
> std::string& result = cache.__emplace();
> assert(&result == &cache.__value() && result == "");
> 
> Cache cache;
> std::string& result = cache.__emplace("foobar");
> assert(&result == &cache.__value() && result == "foobar");
> 
> Cache cache;
> std::string& result = cache.__emplace("foobar", 3);
> assert(&result == &cache.__value() && result == "foo");
> ```
> In this PR, though, it seems more important to test that it's legal to `__emplace` a `NonMovable`, similar to what you did below in the `__emplace_deref` test.
> friend constexpr https://quuxplusone.github.io/blog/2021/04/03/static-constexpr-whittling-knife/ and consider line-breaking somewhere.

Done.

> High-level comment: I'm not sure what benefit you get from the templated operator== or the use of tuple. 

I just want to use any type where I can pass heterogeneous arguments through to the constructor.

> In this PR, though, it seems more important to test that it's legal to `__emplace` a `NonMovable`, similar to what you did below in the `__emplace_deref` test.

It's not valid to do that, though, cause there is no way to perform move elision in that case AFAICT.

================
Comment at: libcxx/test/libcxx/ranges/range.nonprop.cache/emplace_deref.pass.cpp:75
+  // Make sure that we do not require the type to be movable when we emplace-deref it.
+  // Copy elision should be performed instead, see http://eel.is/c++draft/range.nonprop.cache#note-1.
+  {
----------------
miscco wrote:
> Essentially that type will never be copied either so maybe also try an immovable type?
This should really say "Move elision should be performed instead", since that's what is happening. I fixed the comment. The type I'm using (`NonMovable`) *is* immovable. Are you requesting something else that I'm missing?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107932