This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Do not require movability in __non_propagating_cache::__emplace_deref
ClosedPublic

Authored by ldionne on Aug 11 2021, 2:15 PM.

Details

Summary

As explained in http://eel.is/c++draft/range.nonprop.cache#note-1, we
should allow copy and move elision to happen when calling emplace_deref
in non-propagating-cache. Before this change, the only way to emplace
into the non-propagating-cache was to call __set(*it), which materialized
*it when binding it to the reference argument of __set and disabled
move elision.

As a fly-by change, this also renames __set to __emplace for consistency
and adds tests for it.

Diff Detail

Event Timeline

ldionne requested review of this revision.Aug 11 2021, 2:15 PM
ldionne created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2021, 2:15 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added inline comments.
libcxx/include/__ranges/non_propagating_cache.h
46–49

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

86–89

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

libcxx/test/libcxx/ranges/range.nonprop.cache/emplace.pass.cpp
26

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.

libcxx/test/libcxx/ranges/range.nonprop.cache/emplace_deref.pass.cpp
74–82 ↗(On Diff #365851)

(This test LGTM.)

libcxx/test/libcxx/ranges/range.nonprop.cache/has_value.pass.cpp
31

I believe you could delete the characters T{} here, if you wanted.

miscco added a subscriber: miscco.Aug 11 2021, 11:37 PM
miscco added inline comments.
libcxx/include/__ranges/drop_view.h
80

My

libcxx/include/__ranges/non_propagating_cache.h
45

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)

libcxx/test/libcxx/ranges/range.nonprop.cache/emplace_deref.pass.cpp
75 ↗(On Diff #365851)

Essentially that type will never be copied either so maybe also try an immovable type?

libcxx/include/__ranges/non_propagating_cache.h
45

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

94–98

FYI: With a super-eliding __emplace_from, this would turn into

template<class _Fn>
_LIBCPP_HIDE_FROM_ABI
constexpr _Tp& __emplace_from(_Fn __iter) {
  return __value_.emplace(__from_tag{}, _Fn).__t_;
}

template<class _Iter>
_LIBCPP_HIDE_FROM_ABI
constexpr _Tp& __emplace_deref(_Iter const& __iter) {
  return __emplace_from([&]() -> decltype(auto) { return *__iter; });
}
miscco added inline comments.Aug 12 2021, 7:04 AM
libcxx/include/__ranges/non_propagating_cache.h
45

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>

ldionne updated this revision to Diff 366656.Aug 16 2021, 9:10 AM
ldionne marked 11 inline comments as done.

Address review comments

libcxx/include/__ranges/drop_view.h
80

Did you leave this comment by mistake?

libcxx/include/__ranges/non_propagating_cache.h
45

Hmm, I quite like Arthur's idea with a general __emplace_from. It will allow us to easily elide from arbitrary expressions.

86–89

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.

libcxx/test/libcxx/ranges/range.nonprop.cache/emplace.pass.cpp
26

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.

libcxx/test/libcxx/ranges/range.nonprop.cache/emplace_deref.pass.cpp
75 ↗(On Diff #365851)

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?

libcxx/test/libcxx/ranges/range.nonprop.cache/emplace.pass.cpp
26

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);
ldionne updated this revision to Diff 366886.Aug 17 2021, 6:45 AM
ldionne marked an inline comment as done.

Add test requested by Arthur.

libcxx/test/libcxx/ranges/range.nonprop.cache/emplace.pass.cpp
26

Ah, I see what you meant now. Done.

Unlikely to be any more substantial comments from me.

libcxx/include/__ranges/non_propagating_cache.h
103

FWIW, this could be

return __value_.emplace(__from_tag{}, [&]() { return _Tp(_VSTD::forward<_Args>(__args)...); }).__t_;

and then eliminate __forward_tag. No strong preference.

ldionne accepted this revision.Aug 17 2021, 8:32 AM
ldionne marked an inline comment as done.
ldionne added inline comments.
libcxx/include/__ranges/non_propagating_cache.h
103

Will keep as-is, I like to avoid creating a potentially large number of captures in the lambda here (even if those are by ref).

This revision is now accepted and ready to land.Aug 17 2021, 8:32 AM
This revision was automatically updated to reflect the committed changes.
ldionne marked an inline comment as done.