[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/include/__ranges/non_propagating_cache.h:46-49
+      template<class ..._Args>
+      constexpr explicit __wrapper(_Args&& ...__args) : __t_(_VSTD::forward<_Args>(__args)...) { }
+      template<class _Iter>
+      constexpr explicit __wrapper(__deref_tag, _Iter const& __iter) : __t_(*__iter) { }
----------------
This overload set has the distinct smell of "greedy perfect-forwarding overload eats things you wouldn't expect." You use it below like this:
```
    constexpr _Tp& __emplace_deref(_Iter const& __iter) {
      return __value_.emplace(__deref_tag{}, __iter).__t_;
    }
```
which I think is fine; but if you (or future-you) changes it to
```
    constexpr _Tp& __emplace_deref(_Iter __iter) {
      return __value_.emplace(__deref_tag{}, __iter).__t_;
    }
```
then suddenly it'll pick the wrong overload. Right?
I'm not 100% sure this is worth fixing, but if it is, then I think I'd fix it by adding a second tag type, say `struct __no_deref_tag { }`, and making the constructor on line 47 take `__wrapper(__no_deref_tag, _Args&&...)`.


================
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_; }
----------------
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... ;))


================
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; }
+};
----------------
`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.


================
Comment at: libcxx/test/libcxx/ranges/range.nonprop.cache/emplace_deref.pass.cpp:74-82
+  // 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.
+  {
+    using Cache = std::ranges::__non_propagating_cache<NonMovable>;
+    Cache cache;
+    NonMovable& result = cache.__emplace_deref(on_the_fly_iterator_to<NonMovable>{3});
+    assert(&result == &*cache);
----------------
(This test LGTM.)


================
Comment at: libcxx/test/libcxx/ranges/range.nonprop.cache/has_value.pass.cpp:32
   {
-    Cache cache; cache.__set(T{});
+    Cache cache; cache.__emplace(T{});
     assert(cache.__has_value());
----------------
I believe you could delete the characters `T{}` here, if you wanted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107932