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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone 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; }
+};
----------------
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);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107932