Skip to content

Commit dda2349

Browse files
Francesco Zoffolifacebook-github-bot
Francesco Zoffoli
authored andcommitted
Make visit_union take the callable by universal reference
Summary: `visit_union` is taking the callable by value, forcing a copy. This is a problem because 1. it's inconsistent with `std::visit`, which instead uses universal references. This results in surprises when using it 2. when the callable returns a coroutine, this guarantees that the coroutine state is destoryed before the coroutine finish execution ``` int a = 0 co_await thrift::visit_union(my_union, [&](auto, auto) -> Task<void> { a += 1; co_return; }); ``` This results in a dangling reference in the old implementation, and it's correct in the new one. This is consistent with the coro wiki: https://www.internalfb.com/wiki/Coro/#lambdas Note: To be consistent the same should probably be done to also `visit_by_thrift_field_metadata` and the other visit functions Triggering problem: https://fb.workplace.com/groups/192968587972839/posts/1656198051649878/?comment_id=1657068398229510&reply_comment_id=1657106148225735 Reviewed By: Mizuchi Differential Revision: D68151611 fbshipit-source-id: a05c53043642b4274dbab63316e6dcb7d21de9f4
1 parent 8a71f98 commit dda2349

File tree

2 files changed

+6
-4
lines changed

2 files changed

+6
-4
lines changed

third-party/thrift/src/thrift/lib/cpp2/visitation/visit_union.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,8 @@ struct VisitUnion {
4444
* @param f a callable that accepts all member types from union
4545
*/
4646
template <typename T, typename F>
47-
decltype(auto) visit_union(T&& t, F f) {
47+
decltype(auto) visit_union(T&& t, F&& f) {
4848
return apache::thrift::detail::VisitUnion<folly::remove_cvref_t<T>>()(
49-
detail::MetadataForwarder<T, F>{std::move(f)}, static_cast<T&&>(t));
49+
detail::MetadataForwarder<T, F>{std::forward<F>(f)}, static_cast<T&&>(t));
5050
}
5151
} // namespace apache::thrift

third-party/thrift/src/thrift/test/reflection/visitation_visit_union_test.cpp

+4-2
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ struct VisitUnionAdapter {
3737

3838
struct ForEachFieldAdapter {
3939
template <class T, class F>
40-
void operator()(T&& t, F f) const {
40+
void operator()(T&& t, F&& f) const {
4141
apache::thrift::for_each_field(
4242
std::forward<T>(t), [&](auto&& meta, auto&& ref) {
4343
if (folly::to_underlying(t.getType()) == meta.id_ref()) {
@@ -148,10 +148,12 @@ TYPED_TEST(VisitUnionTest, PassCallableByReference) {
148148
TestPassCallableByValue f;
149149
Basic a;
150150
a.int64_ref() = 42;
151-
TestFixture::adapter(a, f);
151+
TestFixture::adapter(a, folly::copy(f));
152152
EXPECT_EQ(f.i, 0);
153153
TestFixture::adapter(a, std::ref(f));
154154
EXPECT_EQ(f.i, 1);
155+
TestFixture::adapter(a, f);
156+
EXPECT_EQ(f.i, 2);
155157
}
156158

157159
template <class T>

0 commit comments

Comments
 (0)