From 334a191c19e565b99f2755b455b937748645d588 Mon Sep 17 00:00:00 2001 From: Shai Szulanski Date: Fri, 10 Jun 2022 13:34:21 -0700 Subject: [PATCH] Make clearing a shared_mutable ref field consistent Summary: The behavior for clearing mutable and const shared ref fields, and optional and unqualified mutable shared ref fields, should be the same, and should not affect copies of the struct. Reviewed By: Mizuchi Differential Revision: D37048487 fbshipit-source-id: fa48ebd2f7d1f3a34e5556a194071ecc843b6cfb --- .../cpp2/module_types_cpp/clear_fields.mustache | 15 ++++----------- .../mcpp2-compare/gen-cpp2/module_types.cpp | 4 ++-- .../gen-cpp2/module_types.cpp | 2 +- thrift/test/ClearTest.cpp | 13 +++++++++++++ thrift/test/clear.thrift | 2 ++ 5 files changed, 22 insertions(+), 14 deletions(-) diff --git a/thrift/compiler/generate/templates/cpp2/module_types_cpp/clear_fields.mustache b/thrift/compiler/generate/templates/cpp2/module_types_cpp/clear_fields.mustache index cfa090a37f2..e8040ff6430 100644 --- a/thrift/compiler/generate/templates/cpp2/module_types_cpp/clear_fields.mustache +++ b/thrift/compiler/generate/templates/cpp2/module_types_cpp/clear_fields.mustache @@ -62,22 +62,15 @@ ::apache::thrift::clear(<% > field/member%>); <%/field:cpp_first_adapter%> <%/field:cpp_ref?%> -<%#field:cpp_ref_unique?%> +<%#field:cpp_ref?%> <%^field:optional?%> +<%#field:cpp_ref_unique?%> if (<% > field/member%>) ::apache::thrift::clear(*<% > field/member%>); -<%/field:optional?%> <%/field:cpp_ref_unique?%> -<%#field:cpp_ref_shared?%> -<%^field:optional?%> - if (<% > field/member%>) ::apache::thrift::clear(*<% > field/member%>); -<%/field:optional?%> -<%/field:cpp_ref_shared?%> -<%#field:cpp_ref_shared_const?%> -<%^field:optional?%> +<%^field:cpp_ref_unique?%> if (<% > field/member%>) <% > field/member%> = ::apache::thrift::detail::make_mutable_smart_ptr<<%field:cpp_storage_type%>>(<%#type:cpp_use_allocator?%>this->get_allocator()<%/type:cpp_use_allocator?%>); +<%/field:cpp_ref_unique?%> <%/field:optional?%> -<%/field:cpp_ref_shared_const?%> -<%#field:cpp_ref?%> <%#field:optional?%> <% > field/member%>.reset(); <%/field:optional?%> diff --git a/thrift/compiler/test/fixtures/mcpp2-compare/gen-cpp2/module_types.cpp b/thrift/compiler/test/fixtures/mcpp2-compare/gen-cpp2/module_types.cpp index 36781cf3ed4..8fdbc3c6109 100644 --- a/thrift/compiler/test/fixtures/mcpp2-compare/gen-cpp2/module_types.cpp +++ b/thrift/compiler/test/fixtures/mcpp2-compare/gen-cpp2/module_types.cpp @@ -2883,9 +2883,9 @@ void AnnotatedStruct::__fbthrift_clear() { this->opt_cpp2_unique_ref.reset(); this->opt_container_with_ref.reset(); if (this->ref_type_unique) ::apache::thrift::clear(*this->ref_type_unique); - if (this->ref_type_shared) ::apache::thrift::clear(*this->ref_type_shared); + if (this->ref_type_shared) this->ref_type_shared = ::apache::thrift::detail::make_mutable_smart_ptr<::std::shared_ptr<::some::valid::ns::containerStruct>>(); this->ref_type_const = ::apache::thrift::detail::make_mutable_smart_ptr<::std::shared_ptr>>>(); - if (this->req_ref_type_shared) ::apache::thrift::clear(*this->req_ref_type_shared); + if (this->req_ref_type_shared) this->req_ref_type_shared = ::apache::thrift::detail::make_mutable_smart_ptr<::std::shared_ptr<::some::valid::ns::containerStruct>>(); if (this->req_ref_type_const) this->req_ref_type_const = ::apache::thrift::detail::make_mutable_smart_ptr<::std::shared_ptr>(); this->req_ref_type_unique = ::apache::thrift::detail::make_mutable_smart_ptr<::std::unique_ptr<::std::vector<::std::string>>>(); this->opt_ref_type_const.reset(); diff --git a/thrift/compiler/test/fixtures/templated-deserialize/gen-cpp2/module_types.cpp b/thrift/compiler/test/fixtures/templated-deserialize/gen-cpp2/module_types.cpp index 4915452c4d7..ae5598f94af 100644 --- a/thrift/compiler/test/fixtures/templated-deserialize/gen-cpp2/module_types.cpp +++ b/thrift/compiler/test/fixtures/templated-deserialize/gen-cpp2/module_types.cpp @@ -349,7 +349,7 @@ void containerStruct::__fbthrift_clear() { this->__fbthrift_field_fieldQ = ::cpp2::MyEnumA(); this->fieldR = ::apache::thrift::detail::make_mutable_smart_ptr<::std::unique_ptr<::std::map<::std::string, bool>>>(); if (this->fieldS) ::apache::thrift::clear(*this->fieldS); - if (this->fieldT) ::apache::thrift::clear(*this->fieldT); + if (this->fieldT) this->fieldT = ::apache::thrift::detail::make_mutable_smart_ptr<::std::shared_ptr<::cpp2::SmallStruct>>(); if (this->fieldU) this->fieldU = ::apache::thrift::detail::make_mutable_smart_ptr<::std::shared_ptr>(); if (this->fieldX) ::apache::thrift::clear(*this->fieldX); __isset = {}; diff --git a/thrift/test/ClearTest.cpp b/thrift/test/ClearTest.cpp index cd90e554641..630acd9080d 100644 --- a/thrift/test/ClearTest.cpp +++ b/thrift/test/ClearTest.cpp @@ -49,6 +49,19 @@ TEST(ClearTest, StructWithDefaultStruct) { checkIsDefault(obj); } +TEST(ClearTest, RefFields) { + StructWithNoDefaultStruct obj; + obj.ref_field_ref()->int_field() = 42; + + auto obj2 = obj; + EXPECT_EQ(*obj.ref_field_ref()->int_field(), 42); + EXPECT_EQ(*obj2.ref_field_ref()->int_field(), 42); + + apache::thrift::clear(obj); + EXPECT_EQ(*obj.ref_field_ref()->int_field(), 0); + EXPECT_EQ(*obj2.ref_field_ref()->int_field(), 42); +} + TEST(AdaptTest, ThriftClearTestStruct) { static_assert(!folly::is_detected_v< adapt_detail::ClearType, diff --git a/thrift/test/clear.thrift b/thrift/test/clear.thrift index a12297004b7..b67164e231d 100644 --- a/thrift/test/clear.thrift +++ b/thrift/test/clear.thrift @@ -48,6 +48,8 @@ struct StructWithNoDefaultStruct { 12: set set_field; 13: map map_field; 14: MyStruct struct_field; + @cpp.Ref{type = cpp.RefType.SharedMutable} + 15: MyStruct ref_field; } struct StructWithDefaultStruct {