Skip to content

Commit

Permalink
Make visit_union take the callable by universal reference
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Francesco Zoffoli authored and facebook-github-bot committed Jan 16, 2025
1 parent 3977301 commit 82eb816
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 4 deletions.
4 changes: 2 additions & 2 deletions thrift/lib/cpp2/visitation/visit_union.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ struct VisitUnion {
* @param f a callable that accepts all member types from union
*/
template <typename T, typename F>
decltype(auto) visit_union(T&& t, F f) {
decltype(auto) visit_union(T&& t, F&& f) {
return apache::thrift::detail::VisitUnion<folly::remove_cvref_t<T>>()(
detail::MetadataForwarder<T, F>{std::move(f)}, static_cast<T&&>(t));
detail::MetadataForwarder<T, F>{std::forward<F>(f)}, static_cast<T&&>(t));
}
} // namespace apache::thrift
6 changes: 4 additions & 2 deletions thrift/test/reflection/visitation_visit_union_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ struct VisitUnionAdapter {

struct ForEachFieldAdapter {
template <class T, class F>
void operator()(T&& t, F f) const {
void operator()(T&& t, F&& f) const {
apache::thrift::for_each_field(
std::forward<T>(t), [&](auto&& meta, auto&& ref) {
if (folly::to_underlying(t.getType()) == meta.id_ref()) {
Expand Down Expand Up @@ -148,10 +148,12 @@ TYPED_TEST(VisitUnionTest, PassCallableByReference) {
TestPassCallableByValue f;
Basic a;
a.int64_ref() = 42;
TestFixture::adapter(a, f);
TestFixture::adapter(a, folly::copy(f));
EXPECT_EQ(f.i, 0);
TestFixture::adapter(a, std::ref(f));
EXPECT_EQ(f.i, 1);
TestFixture::adapter(a, f);
EXPECT_EQ(f.i, 2);
}

template <class T>
Expand Down

0 comments on commit 82eb816

Please sign in to comment.