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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
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:
> ldionne wrote:
> > 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.
> > In this PR, though, it seems more important to test that it's legal to `__emplace` a `NonMovable`
> ```
> std::ranges::__non_propagating_cache<NonMovable> cache;
> NonMovable& result = cache.__emplace();
> assert(&result == &*cache);
> ```
Ah, I see what you meant now. Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107932