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
facebook · Jun 10, 2022 · 334a191 · 334a191
1 parent 047bc0e
commit 334a191
Show file tree

Hide file tree

Showing 5 changed files with 22 additions and 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
@@ -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
@@ -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<const ::std::map<::std::int32_t, ::std::vector<::std::string>>>>();
-  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<const ::some::valid::ns::containerStruct>>();
   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
@@ -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<const ::cpp2::SmallStruct>>();
   if (this->fieldX) ::apache::thrift::clear(*this->fieldX);
   __isset = {};

diff --git 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
@@ -48,6 +48,8 @@ struct StructWithNoDefaultStruct {
   12: set<i16> set_field;
   13: map<i16, i16> map_field;
   14: MyStruct struct_field;
+  @cpp.Ref{type = cpp.RefType.SharedMutable}
+  15: MyStruct ref_field;
 }
 
 struct StructWithDefaultStruct {