From b90fa7f01ec2ae227b9675535b969cf571eea3d9 Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Tue, 3 Sep 2024 13:41:47 -0700 Subject: [PATCH] Streamline creation of new elements in `RepeatedPtrField` The logic to create new elements in `RepeatedPtrField` was spread across an inconsistent patchwork of specializations of individual `GenericTypeHandler` static functions as well as external member and non-member functions, all living in multiple sources. Now all that logic is concentrated in three `GenericTypeHandler` full class specializations. One change that to carefully verify in postsubmit is the now-removed old workaround for an MSVC bug #254 (MSVC compatibility is not checked in presubmit). PiperOrigin-RevId: 670689695 --- src/google/protobuf/arena.h | 11 + src/google/protobuf/arenastring.h | 28 +-- src/google/protobuf/compiler/cpp/message.cc | 6 +- .../protobuf/compiler/java/java_features.pb.h | 6 +- src/google/protobuf/compiler/plugin.pb.h | 24 +- src/google/protobuf/cpp_features.pb.h | 6 +- src/google/protobuf/descriptor.pb.h | 198 +++------------ src/google/protobuf/extension_set.cc | 10 +- .../protobuf/generated_message_reflection.cc | 24 +- src/google/protobuf/lazy_repeated_field.cc | 21 +- src/google/protobuf/lazy_repeated_field.h | 3 +- src/google/protobuf/message.cc | 22 -- src/google/protobuf/message_lite.cc | 10 - src/google/protobuf/message_lite.h | 5 - src/google/protobuf/port.h | 4 + src/google/protobuf/port_def.inc | 4 - src/google/protobuf/port_undef.inc | 1 - src/google/protobuf/reflection_ops.cc | 22 +- src/google/protobuf/repeated_field.h | 13 +- .../protobuf/repeated_field_unittest.cc | 2 - src/google/protobuf/repeated_ptr_field.cc | 19 +- src/google/protobuf/repeated_ptr_field.h | 228 +++++++++--------- 22 files changed, 218 insertions(+), 449 deletions(-) diff --git a/src/google/protobuf/arena.h b/src/google/protobuf/arena.h index a853034140dac..6b6ff37d72148 100644 --- a/src/google/protobuf/arena.h +++ b/src/google/protobuf/arena.h @@ -87,6 +87,17 @@ template void arena_delete_object(void* object) { delete reinterpret_cast(object); } + +inline bool CanUseInternalSwap(Arena* lhs, Arena* rhs) { + if (DebugHardenForceCopyInSwap()) { + // We force copy in swap when we are not using an arena. + // If we did with an arena we would grow arena usage too much. + return lhs != nullptr && lhs == rhs; + } else { + return lhs == rhs; + } +} + } // namespace internal // ArenaOptions provides optional additional parameters to arena construction diff --git a/src/google/protobuf/arenastring.h b/src/google/protobuf/arenastring.h index 0c1c4e271dd7a..1267b4cfb8050 100644 --- a/src/google/protobuf/arenastring.h +++ b/src/google/protobuf/arenastring.h @@ -503,22 +503,22 @@ inline PROTOBUF_NDEBUG_INLINE void ArenaStringPtr::InternalSwap( // Silence unused variable warnings in release buildls. (void)arena; std::swap(lhs->tagged_ptr_, rhs->tagged_ptr_); -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - for (auto* p : {lhs, rhs}) { - if (p->IsDefault()) continue; - std::string* old_value = p->tagged_ptr_.Get(); - std::string* new_value = - p->IsFixedSizeArena() - ? Arena::Create(arena, *old_value) - : Arena::Create(arena, std::move(*old_value)); - if (arena == nullptr) { - delete old_value; - p->tagged_ptr_.SetAllocated(new_value); - } else { - p->tagged_ptr_.SetMutableArena(new_value); + if (internal::DebugHardenForceCopyInSwap()) { + for (auto* p : {lhs, rhs}) { + if (p->IsDefault()) continue; + std::string* old_value = p->tagged_ptr_.Get(); + std::string* new_value = + p->IsFixedSizeArena() + ? Arena::Create(arena, *old_value) + : Arena::Create(arena, std::move(*old_value)); + if (arena == nullptr) { + delete old_value; + p->tagged_ptr_.SetAllocated(new_value); + } else { + p->tagged_ptr_.SetMutableArena(new_value); + } } } -#endif // PROTOBUF_FORCE_COPY_IN_SWAP } inline void ArenaStringPtr::ClearNonDefaultToEmpty() { diff --git a/src/google/protobuf/compiler/cpp/message.cc b/src/google/protobuf/compiler/cpp/message.cc index 0901ac55b9ccf..547b5a6154266 100644 --- a/src/google/protobuf/compiler/cpp/message.cc +++ b/src/google/protobuf/compiler/cpp/message.cc @@ -2126,11 +2126,7 @@ void MessageGenerator::GenerateClassDefinition(io::Printer* p) { friend void swap($classname$& a, $classname$& b) { a.Swap(&b); } inline void Swap($classname$* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if ($pbi$::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { $pbi$::GenericSwap(this, other); diff --git a/src/google/protobuf/compiler/java/java_features.pb.h b/src/google/protobuf/compiler/java/java_features.pb.h index 707a65f692b44..87d53bca49a56 100644 --- a/src/google/protobuf/compiler/java/java_features.pb.h +++ b/src/google/protobuf/compiler/java/java_features.pb.h @@ -159,11 +159,7 @@ class PROTOC_EXPORT JavaFeatures final friend void swap(JavaFeatures& a, JavaFeatures& b) { a.Swap(&b); } inline void Swap(JavaFeatures* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); diff --git a/src/google/protobuf/compiler/plugin.pb.h b/src/google/protobuf/compiler/plugin.pb.h index b6ab93d9bbe77..f47ee739a66b3 100644 --- a/src/google/protobuf/compiler/plugin.pb.h +++ b/src/google/protobuf/compiler/plugin.pb.h @@ -176,11 +176,7 @@ class PROTOC_EXPORT Version final friend void swap(Version& a, Version& b) { a.Swap(&b); } inline void Swap(Version* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -415,11 +411,7 @@ class PROTOC_EXPORT CodeGeneratorResponse_File final friend void swap(CodeGeneratorResponse_File& a, CodeGeneratorResponse_File& b) { a.Swap(&b); } inline void Swap(CodeGeneratorResponse_File* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -670,11 +662,7 @@ class PROTOC_EXPORT CodeGeneratorResponse final friend void swap(CodeGeneratorResponse& a, CodeGeneratorResponse& b) { a.Swap(&b); } inline void Swap(CodeGeneratorResponse* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -949,11 +937,7 @@ class PROTOC_EXPORT CodeGeneratorRequest final friend void swap(CodeGeneratorRequest& a, CodeGeneratorRequest& b) { a.Swap(&b); } inline void Swap(CodeGeneratorRequest* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); diff --git a/src/google/protobuf/cpp_features.pb.h b/src/google/protobuf/cpp_features.pb.h index de4cc6be4772c..86812176c2abf 100644 --- a/src/google/protobuf/cpp_features.pb.h +++ b/src/google/protobuf/cpp_features.pb.h @@ -160,11 +160,7 @@ class PROTOBUF_EXPORT CppFeatures final friend void swap(CppFeatures& a, CppFeatures& b) { a.Swap(&b); } inline void Swap(CppFeatures* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); diff --git a/src/google/protobuf/descriptor.pb.h b/src/google/protobuf/descriptor.pb.h index 723f20db29d4f..a33bf456b7ce8 100644 --- a/src/google/protobuf/descriptor.pb.h +++ b/src/google/protobuf/descriptor.pb.h @@ -762,11 +762,7 @@ class PROTOBUF_EXPORT UninterpretedOption_NamePart final friend void swap(UninterpretedOption_NamePart& a, UninterpretedOption_NamePart& b) { a.Swap(&b); } inline void Swap(UninterpretedOption_NamePart* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -980,11 +976,7 @@ class PROTOBUF_EXPORT SourceCodeInfo_Location final friend void swap(SourceCodeInfo_Location& a, SourceCodeInfo_Location& b) { a.Swap(&b); } inline void Swap(SourceCodeInfo_Location* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -1265,11 +1257,7 @@ class PROTOBUF_EXPORT GeneratedCodeInfo_Annotation final friend void swap(GeneratedCodeInfo_Annotation& a, GeneratedCodeInfo_Annotation& b) { a.Swap(&b); } inline void Swap(GeneratedCodeInfo_Annotation* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -1545,11 +1533,7 @@ class PROTOBUF_EXPORT FieldOptions_FeatureSupport final friend void swap(FieldOptions_FeatureSupport& a, FieldOptions_FeatureSupport& b) { a.Swap(&b); } inline void Swap(FieldOptions_FeatureSupport* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -1784,11 +1768,7 @@ class PROTOBUF_EXPORT FieldOptions_EditionDefault final friend void swap(FieldOptions_EditionDefault& a, FieldOptions_EditionDefault& b) { a.Swap(&b); } inline void Swap(FieldOptions_EditionDefault* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -1997,11 +1977,7 @@ class PROTOBUF_EXPORT FeatureSet final friend void swap(FeatureSet& a, FeatureSet& b) { a.Swap(&b); } inline void Swap(FeatureSet* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -2562,11 +2538,7 @@ class PROTOBUF_EXPORT ExtensionRangeOptions_Declaration final friend void swap(ExtensionRangeOptions_Declaration& a, ExtensionRangeOptions_Declaration& b) { a.Swap(&b); } inline void Swap(ExtensionRangeOptions_Declaration* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -2820,11 +2792,7 @@ class PROTOBUF_EXPORT EnumDescriptorProto_EnumReservedRange final friend void swap(EnumDescriptorProto_EnumReservedRange& a, EnumDescriptorProto_EnumReservedRange& b) { a.Swap(&b); } inline void Swap(EnumDescriptorProto_EnumReservedRange* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -3027,11 +2995,7 @@ class PROTOBUF_EXPORT DescriptorProto_ReservedRange final friend void swap(DescriptorProto_ReservedRange& a, DescriptorProto_ReservedRange& b) { a.Swap(&b); } inline void Swap(DescriptorProto_ReservedRange* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -3234,11 +3198,7 @@ class PROTOBUF_EXPORT UninterpretedOption final friend void swap(UninterpretedOption& a, UninterpretedOption& b) { a.Swap(&b); } inline void Swap(UninterpretedOption* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -3536,11 +3496,7 @@ class PROTOBUF_EXPORT SourceCodeInfo final friend void swap(SourceCodeInfo& a, SourceCodeInfo& b) { a.Swap(&b); } inline void Swap(SourceCodeInfo* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -3736,11 +3692,7 @@ class PROTOBUF_EXPORT GeneratedCodeInfo final friend void swap(GeneratedCodeInfo& a, GeneratedCodeInfo& b) { a.Swap(&b); } inline void Swap(GeneratedCodeInfo* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -3936,11 +3888,7 @@ class PROTOBUF_EXPORT FeatureSetDefaults_FeatureSetEditionDefault final friend void swap(FeatureSetDefaults_FeatureSetEditionDefault& a, FeatureSetDefaults_FeatureSetEditionDefault& b) { a.Swap(&b); } inline void Swap(FeatureSetDefaults_FeatureSetEditionDefault* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -4169,11 +4117,7 @@ class PROTOBUF_EXPORT ServiceOptions final friend void swap(ServiceOptions& a, ServiceOptions& b) { a.Swap(&b); } inline void Swap(ServiceOptions* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -4584,11 +4528,7 @@ class PROTOBUF_EXPORT OneofOptions final friend void swap(OneofOptions& a, OneofOptions& b) { a.Swap(&b); } inline void Swap(OneofOptions* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -4986,11 +4926,7 @@ class PROTOBUF_EXPORT MethodOptions final friend void swap(MethodOptions& a, MethodOptions& b) { a.Swap(&b); } inline void Swap(MethodOptions* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -5434,11 +5370,7 @@ class PROTOBUF_EXPORT MessageOptions final friend void swap(MessageOptions& a, MessageOptions& b) { a.Swap(&b); } inline void Swap(MessageOptions* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -5901,11 +5833,7 @@ class PROTOBUF_EXPORT FileOptions final friend void swap(FileOptions& a, FileOptions& b) { a.Swap(&b); } inline void Swap(FileOptions* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -6630,11 +6558,7 @@ class PROTOBUF_EXPORT FieldOptions final friend void swap(FieldOptions& a, FieldOptions& b) { a.Swap(&b); } inline void Swap(FieldOptions* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -7295,11 +7219,7 @@ class PROTOBUF_EXPORT FeatureSetDefaults final friend void swap(FeatureSetDefaults& a, FeatureSetDefaults& b) { a.Swap(&b); } inline void Swap(FeatureSetDefaults* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -7527,11 +7447,7 @@ class PROTOBUF_EXPORT ExtensionRangeOptions final friend void swap(ExtensionRangeOptions& a, ExtensionRangeOptions& b) { a.Swap(&b); } inline void Swap(ExtensionRangeOptions* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -7981,11 +7897,7 @@ class PROTOBUF_EXPORT EnumValueOptions final friend void swap(EnumValueOptions& a, EnumValueOptions& b) { a.Swap(&b); } inline void Swap(EnumValueOptions* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -8426,11 +8338,7 @@ class PROTOBUF_EXPORT EnumOptions final friend void swap(EnumOptions& a, EnumOptions& b) { a.Swap(&b); } inline void Swap(EnumOptions* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -8867,11 +8775,7 @@ class PROTOBUF_EXPORT OneofDescriptorProto final friend void swap(OneofDescriptorProto& a, OneofDescriptorProto& b) { a.Swap(&b); } inline void Swap(OneofDescriptorProto* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -9089,11 +8993,7 @@ class PROTOBUF_EXPORT MethodDescriptorProto final friend void swap(MethodDescriptorProto& a, MethodDescriptorProto& b) { a.Swap(&b); } inline void Swap(MethodDescriptorProto* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -9375,11 +9275,7 @@ class PROTOBUF_EXPORT FieldDescriptorProto final friend void swap(FieldDescriptorProto& a, FieldDescriptorProto& b) { a.Swap(&b); } inline void Swap(FieldDescriptorProto* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -9793,11 +9689,7 @@ class PROTOBUF_EXPORT EnumValueDescriptorProto final friend void swap(EnumValueDescriptorProto& a, EnumValueDescriptorProto& b) { a.Swap(&b); } inline void Swap(EnumValueDescriptorProto* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -10028,11 +9920,7 @@ class PROTOBUF_EXPORT DescriptorProto_ExtensionRange final friend void swap(DescriptorProto_ExtensionRange& a, DescriptorProto_ExtensionRange& b) { a.Swap(&b); } inline void Swap(DescriptorProto_ExtensionRange* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -10257,11 +10145,7 @@ class PROTOBUF_EXPORT ServiceDescriptorProto final friend void swap(ServiceDescriptorProto& a, ServiceDescriptorProto& b) { a.Swap(&b); } inline void Swap(ServiceDescriptorProto* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -10498,11 +10382,7 @@ class PROTOBUF_EXPORT EnumDescriptorProto final friend void swap(EnumDescriptorProto& a, EnumDescriptorProto& b) { a.Swap(&b); } inline void Swap(EnumDescriptorProto* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -10783,11 +10663,7 @@ class PROTOBUF_EXPORT DescriptorProto final friend void swap(DescriptorProto& a, DescriptorProto& b) { a.Swap(&b); } inline void Swap(DescriptorProto* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -11164,11 +11040,7 @@ class PROTOBUF_EXPORT FileDescriptorProto final friend void swap(FileDescriptorProto& a, FileDescriptorProto& b) { a.Swap(&b); } inline void Swap(FileDescriptorProto* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); @@ -11594,11 +11466,7 @@ class PROTOBUF_EXPORT FileDescriptorSet final friend void swap(FileDescriptorSet& a, FileDescriptorSet& b) { a.Swap(&b); } inline void Swap(FileDescriptorSet* other) { if (other == this) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (::google::protobuf::internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { ::google::protobuf::internal::GenericSwap(this, other); diff --git a/src/google/protobuf/extension_set.cc b/src/google/protobuf/extension_set.cc index a673e0bf4e8e4..3665660e4e2a8 100644 --- a/src/google/protobuf/extension_set.cc +++ b/src/google/protobuf/extension_set.cc @@ -811,9 +811,7 @@ MessageLite* ExtensionSet::AddMessage(int number, FieldType type, ABSL_DCHECK_TYPE(*extension, REPEATED_FIELD, MESSAGE); } - return reinterpret_cast( - extension->ptr.repeated_message_value) - ->AddMessage(&prototype); + return extension->ptr.repeated_message_value->AddFromPrototype(&prototype); } // Defined in extension_set_heavy.cc. @@ -1097,11 +1095,7 @@ void ExtensionSet::InternalExtensionMergeFrom(const MessageLite* extendee, } void ExtensionSet::Swap(const MessageLite* extendee, ExtensionSet* other) { -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (arena_ != nullptr && arena_ == other->arena_) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (arena_ == other->arena_) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (internal::CanUseInternalSwap(arena_, other->arena_)) { InternalSwap(other); } else { // TODO: We maybe able to optimize a case where we are diff --git a/src/google/protobuf/generated_message_reflection.cc b/src/google/protobuf/generated_message_reflection.cc index 5c7a86d4ac92f..c657c189f6eb9 100644 --- a/src/google/protobuf/generated_message_reflection.cc +++ b/src/google/protobuf/generated_message_reflection.cc @@ -779,11 +779,7 @@ void SwapFieldHelper::SwapMessage(const Reflection* r, Message* lhs, if (*lhs_sub == *rhs_sub) return; -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (lhs_arena != nullptr && lhs_arena == rhs_arena) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (lhs_arena == rhs_arena) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (internal::CanUseInternalSwap(lhs_arena, rhs_arena)) { std::swap(*lhs_sub, *rhs_sub); return; } @@ -1084,11 +1080,7 @@ void Reflection::Swap(Message* lhs, Message* rhs) const { // Check that both messages are in the same arena (or both on the heap). We // need to copy all data if not, due to ownership semantics. -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (lhs_arena == nullptr || lhs_arena != rhs_arena) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (lhs_arena != rhs_arena) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (!internal::CanUseInternalSwap(lhs_arena, rhs_arena)) { // One of the two is guaranteed to have an arena. Switch things around // to guarantee that lhs has an arena. Arena* arena = lhs_arena; @@ -1100,12 +1092,12 @@ void Reflection::Swap(Message* lhs, Message* rhs) const { Message* temp = lhs->New(arena); temp->MergeFrom(*rhs); rhs->CopyFrom(*lhs); -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - lhs->CopyFrom(*temp); - if (arena == nullptr) delete temp; -#else // PROTOBUF_FORCE_COPY_IN_SWAP - Swap(lhs, temp); -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (internal::DebugHardenForceCopyInSwap()) { + lhs->CopyFrom(*temp); + if (arena == nullptr) delete temp; + } else { + Swap(lhs, temp); + } return; } diff --git a/src/google/protobuf/lazy_repeated_field.cc b/src/google/protobuf/lazy_repeated_field.cc index e3b83ad04dcb5..4ea2a504cf549 100644 --- a/src/google/protobuf/lazy_repeated_field.cc +++ b/src/google/protobuf/lazy_repeated_field.cc @@ -27,11 +27,12 @@ #include "google/protobuf/io/zero_copy_stream_impl_lite.h" #include "google/protobuf/message_lite.h" #include "google/protobuf/parse_context.h" +#include "google/protobuf/port.h" +#include "google/protobuf/repeated_ptr_field.h" // Must be included last. // clang-format off #include "google/protobuf/port_def.inc" -#include "google/protobuf/repeated_ptr_field.h" // clang-format on namespace google { @@ -222,13 +223,13 @@ void LazyRepeatedPtrField::Swap(LazyRepeatedPtrField* lhs, Arena* lhs_arena, if (cleanup_old) old_unparsed.Destroy(); }; static auto take_ownership = [](LazyRepeatedPtrField* f, Arena* arena) { -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - reallocate(f, arena, true); -#else - arena->Own(reinterpret_cast*>( - f->raw_.load(std::memory_order_relaxed).mutable_value())); - f->unparsed_.TransferHeapOwnershipToArena(arena); -#endif + if (internal::DebugHardenForceCopyInSwap()) { + reallocate(f, arena, true); + } else { + arena->Own(reinterpret_cast*>( + f->raw_.load(std::memory_order_relaxed).mutable_value())); + f->unparsed_.TransferHeapOwnershipToArena(arena); + } }; using std::swap; // Enable ADL with fallback @@ -239,12 +240,10 @@ void LazyRepeatedPtrField::Swap(LazyRepeatedPtrField* lhs, Arena* lhs_arena, // arena is actually on the opposite message. Now we straighten out our // ownership by forcing reallocations/ownership changes as needed. if (lhs_arena == rhs_arena) { -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (lhs_arena == nullptr) { + if (internal::DebugHardenForceCopyInSwap() && lhs_arena == nullptr) { reallocate(lhs, lhs_arena, true); reallocate(rhs, rhs_arena, true); } -#endif } else { if (lhs_arena == nullptr) { take_ownership(rhs, rhs_arena); diff --git a/src/google/protobuf/lazy_repeated_field.h b/src/google/protobuf/lazy_repeated_field.h index ab573e58c0810..40ec02e771aa4 100644 --- a/src/google/protobuf/lazy_repeated_field.h +++ b/src/google/protobuf/lazy_repeated_field.h @@ -328,7 +328,8 @@ class LazyRepeatedPtrField { const char* ptr2 = ptr; TagType next_tag; do { - MessageLite* submsg = value->AddMessage(prototype); + MessageLite* submsg = + value->AddFromPrototype>(prototype); // ptr2 points to the start of the element's encoded length. ptr = ctx->ParseMessage(submsg, ptr2); if (PROTOBUF_PREDICT_FALSE(ptr == nullptr)) return nullptr; diff --git a/src/google/protobuf/message.cc b/src/google/protobuf/message.cc index f60818f424b11..40afaa6e63c1d 100644 --- a/src/google/protobuf/message.cc +++ b/src/google/protobuf/message.cc @@ -495,28 +495,6 @@ const internal::RepeatedFieldAccessor* Reflection::RepeatedFieldAccessor( } namespace internal { -template <> -#if defined(_MSC_VER) && (_MSC_VER >= 1800) -// Note: force noinline to workaround MSVC compiler bug with /Zc:inline, issue -// #240 -PROTOBUF_NOINLINE -#endif - Message* - GenericTypeHandler::NewFromPrototype(const Message* prototype, - Arena* arena) { - return prototype->New(arena); -} -template <> -#if defined(_MSC_VER) && (_MSC_VER >= 1800) -// Note: force noinline to workaround MSVC compiler bug with /Zc:inline, issue -// #240 -PROTOBUF_NOINLINE -#endif - Arena* - GenericTypeHandler::GetArena(Message* value) { - return value->GetArena(); -} - template void InternalMetadata::DoClear(); template void InternalMetadata::DoMergeFrom( const UnknownFieldSet& other); diff --git a/src/google/protobuf/message_lite.cc b/src/google/protobuf/message_lite.cc index 9b3debf11e850..acc9859130930 100644 --- a/src/google/protobuf/message_lite.cc +++ b/src/google/protobuf/message_lite.cc @@ -703,16 +703,6 @@ absl::Cord MessageLite::SerializePartialAsCord() const { namespace internal { -MessageLite* NewFromPrototypeHelper(const MessageLite* prototype, - Arena* arena) { - return prototype->New(arena); -} -template <> -void GenericTypeHandler::Merge(const MessageLite& from, - MessageLite* to) { - to->CheckTypeAndMergeFrom(from); -} - // Non-inline variants of std::string specializations for // various InternalMetadata routines. template <> diff --git a/src/google/protobuf/message_lite.h b/src/google/protobuf/message_lite.h index c3ae2039b3d63..571e2f535bd9f 100644 --- a/src/google/protobuf/message_lite.h +++ b/src/google/protobuf/message_lite.h @@ -225,9 +225,6 @@ class WireFormatLite; class WeakFieldMap; class RustMapHelper; -template -class GenericTypeHandler; // defined in repeated_field.h - // We compute sizes as size_t but cache them as int. This function converts a // computed size to a cached size. Since we don't proceed with serialization // if the total size was > INT_MAX, it is not important what this function @@ -963,8 +960,6 @@ class PROTOBUF_EXPORT MessageLite { template friend class Arena::InternalHelper; - template - friend class internal::GenericTypeHandler; friend auto internal::GetClassData(const MessageLite& msg); diff --git a/src/google/protobuf/port.h b/src/google/protobuf/port.h index 9cfc2806e8802..a011b7383ff82 100644 --- a/src/google/protobuf/port.h +++ b/src/google/protobuf/port.h @@ -276,6 +276,10 @@ constexpr bool DebugHardenForceCopyInRelease() { return false; } +constexpr bool DebugHardenForceCopyInSwap() { + return false; +} + // Returns true if pointers are 8B aligned, leaving least significant 3 bits // available. inline constexpr bool PtrIsAtLeast8BAligned() { return alignof(void*) >= 8; } diff --git a/src/google/protobuf/port_def.inc b/src/google/protobuf/port_def.inc index 78ba5d5e51d23..eb0e0f7b077be 100644 --- a/src/google/protobuf/port_def.inc +++ b/src/google/protobuf/port_def.inc @@ -398,10 +398,6 @@ static_assert(PROTOBUF_ABSL_MIN(20230125, 3), #define PROTOBUF_RESTRICT #endif -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP -#error PROTOBUF_FORCE_COPY_IN_SWAP was previously defined -#endif - #ifdef PROTOBUF_FORCE_COPY_IN_MOVE #error PROTOBUF_FORCE_COPY_IN_MOVE was previously defined #endif diff --git a/src/google/protobuf/port_undef.inc b/src/google/protobuf/port_undef.inc index 381e85ed3c923..8ed957d5857dc 100644 --- a/src/google/protobuf/port_undef.inc +++ b/src/google/protobuf/port_undef.inc @@ -41,7 +41,6 @@ #undef PROTOC_EXPORT #undef PROTOBUF_NODISCARD #undef PROTOBUF_RESTRICT -#undef PROTOBUF_FORCE_COPY_IN_SWAP #undef PROTOBUF_FORCE_COPY_IN_MOVE #undef PROTOBUF_FUZZ_MESSAGE_SPACE_USED_LONG #undef PROTOBUF_FORCE_COPY_DEFAULT_STRING diff --git a/src/google/protobuf/reflection_ops.cc b/src/google/protobuf/reflection_ops.cc index 3b77cf41e0ec9..6d6811becf5c5 100644 --- a/src/google/protobuf/reflection_ops.cc +++ b/src/google/protobuf/reflection_ops.cc @@ -419,10 +419,10 @@ void ReflectionOps::FindInitializationErrors(const Message& message, } void GenericSwap(Message* lhs, Message* rhs) { -#ifndef PROTOBUF_FORCE_COPY_IN_SWAP - ABSL_DCHECK(lhs->GetArena() != rhs->GetArena()); - ABSL_DCHECK(lhs->GetArena() != nullptr || rhs->GetArena() != nullptr); -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (!internal::DebugHardenForceCopyInSwap()) { + ABSL_DCHECK(lhs->GetArena() != rhs->GetArena()); + ABSL_DCHECK(lhs->GetArena() != nullptr || rhs->GetArena() != nullptr); + } // At least one of these must have an arena, so make `rhs` point to it. Arena* arena = rhs->GetArena(); if (arena == nullptr) { @@ -436,13 +436,13 @@ void GenericSwap(Message* lhs, Message* rhs) { tmp->CheckTypeAndMergeFrom(*lhs); lhs->Clear(); lhs->CheckTypeAndMergeFrom(*rhs); -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - rhs->Clear(); - rhs->CheckTypeAndMergeFrom(*tmp); - if (arena == nullptr) delete tmp; -#else // PROTOBUF_FORCE_COPY_IN_SWAP - rhs->GetReflection()->Swap(tmp, rhs); -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (internal::DebugHardenForceCopyInSwap()) { + rhs->Clear(); + rhs->CheckTypeAndMergeFrom(*tmp); + if (arena == nullptr) delete tmp; + } else { + rhs->GetReflection()->Swap(tmp, rhs); + } } } // namespace internal diff --git a/src/google/protobuf/repeated_field.h b/src/google/protobuf/repeated_field.h index 5d13a3f676cdc..f885f49d594bf 100644 --- a/src/google/protobuf/repeated_field.h +++ b/src/google/protobuf/repeated_field.h @@ -28,7 +28,6 @@ #include #include #include -#include #include #include @@ -36,7 +35,6 @@ #include "absl/base/dynamic_annotations.h" #include "absl/base/optimization.h" #include "absl/log/absl_check.h" -#include "absl/log/absl_log.h" #include "absl/meta/type_traits.h" #include "absl/strings/cord.h" #include "google/protobuf/arena.h" @@ -46,7 +44,6 @@ #include "google/protobuf/port.h" #include "google/protobuf/repeated_ptr_field.h" - // Must be included last. #include "google/protobuf/port_def.inc" @@ -79,7 +76,11 @@ constexpr int RepeatedFieldLowerClampLimit() { // overflows when multiplied by 2 (which is undefined behavior). Sizes above // this will clamp to the maximum int value instead of following exponential // growth when growing a repeated field. +#if defined(__cpp_inline_variables) +inline constexpr int kRepeatedFieldUpperClampLimit = +#else constexpr int kRepeatedFieldUpperClampLimit = +#endif (std::numeric_limits::max() / 2) + 1; template @@ -1074,11 +1075,7 @@ void RepeatedField::Swap(RepeatedField* other) { if (this == other) return; Arena* arena = GetArena(); Arena* other_arena = other->GetArena(); -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (arena != nullptr && arena == other_arena) { -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (arena == other_arena) { -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + if (internal::CanUseInternalSwap(arena, other_arena)) { InternalSwap(other); } else { RepeatedField temp(other_arena); diff --git a/src/google/protobuf/repeated_field_unittest.cc b/src/google/protobuf/repeated_field_unittest.cc index 551b135811f74..e7b4e4b9318e7 100644 --- a/src/google/protobuf/repeated_field_unittest.cc +++ b/src/google/protobuf/repeated_field_unittest.cc @@ -19,7 +19,6 @@ #include #include #include -#include #include #include #include @@ -34,7 +33,6 @@ #include #include "absl/log/absl_check.h" #include "absl/numeric/bits.h" -#include "absl/random/random.h" #include "absl/strings/cord.h" #include "absl/strings/str_cat.h" #include "absl/types/span.h" diff --git a/src/google/protobuf/repeated_ptr_field.cc b/src/google/protobuf/repeated_ptr_field.cc index 891439c14d257..6c95d427d502f 100644 --- a/src/google/protobuf/repeated_ptr_field.cc +++ b/src/google/protobuf/repeated_ptr_field.cc @@ -16,9 +16,9 @@ #include #include #include +#include #include -#include "absl/base/prefetch.h" #include "absl/log/absl_check.h" #include "google/protobuf/arena.h" #include "google/protobuf/message_lite.h" @@ -93,14 +93,6 @@ void RepeatedPtrFieldBase::DestroyProtos() { tagged_rep_or_elem_ = nullptr; } -void* RepeatedPtrFieldBase::AddMessageLite(ElementFactory factory) { - return AddInternal(factory); -} - -void* RepeatedPtrFieldBase::AddString() { - return AddInternal([](Arena* arena) { return NewStringElement(arena); }); -} - void RepeatedPtrFieldBase::CloseGap(int start, int num) { if (using_sso()) { if (start == 0 && num == 1) { @@ -116,11 +108,6 @@ void RepeatedPtrFieldBase::CloseGap(int start, int num) { ExchangeCurrentSize(current_size_ - num); } -MessageLite* RepeatedPtrFieldBase::AddMessage(const MessageLite* prototype) { - return static_cast( - AddInternal([prototype](Arena* a) { return prototype->New(a); })); -} - void InternalOutOfLineDeleteMessageLite(MessageLite* message) { delete message; } @@ -223,10 +210,6 @@ void RepeatedPtrFieldBase::MergeFrom( } } -void* NewStringElement(Arena* arena) { - return Arena::Create(arena); -} - } // namespace internal } // namespace protobuf } // namespace google diff --git a/src/google/protobuf/repeated_ptr_field.h b/src/google/protobuf/repeated_ptr_field.h index 9659da94e0ef0..b2178c6bf1f3f 100644 --- a/src/google/protobuf/repeated_ptr_field.h +++ b/src/google/protobuf/repeated_ptr_field.h @@ -26,8 +26,8 @@ #include #include #include +#include #include -#include #include #include @@ -40,7 +40,6 @@ #include "google/protobuf/message_lite.h" #include "google/protobuf/port.h" - // Must be included last. #include "google/protobuf/port_def.inc" @@ -92,9 +91,13 @@ struct IsMovable // Do not use this struct - it exists for internal use only. template struct ArenaOffsetHelper { - constexpr static size_t value = offsetof(T, arena_); + static constexpr size_t value = offsetof(T, arena_); }; +// Defined further below. +template +class GenericTypeHandler; + // This is the common base class for RepeatedPtrFields. It deals only in void* // pointers. Users should not use this interface directly. // @@ -102,11 +105,17 @@ struct ArenaOffsetHelper { // but may have a template argument called TypeHandler. Its signature is: // class TypeHandler { // public: -// typedef MyType Type; -// static Type* New(); -// static Type* NewFromPrototype(const Type* prototype, -// Arena* arena); -// static void Delete(Type*); +// using Type = MyType; +// using Movable = ...; +// +// static ElementFactory GetNewFunc(); +// static ElementFactory GetNewFromPrototypeFunc(const Type* prototype); +// static void GetArena(Type* value); +// +// static Type* New(Arena* arena); +// static Type* New(Arena* arena, Type&& value); +// static Type* NewFromPrototype(const Type* prototype, Arena* arena); +// static void Delete(Type*, Arena* arena); // static void Clear(Type*); // static void Merge(const Type& from, Type* to); // @@ -114,20 +123,18 @@ struct ArenaOffsetHelper { // static int SpaceUsedLong(const Type&); // }; class PROTOBUF_EXPORT RepeatedPtrFieldBase { - template - using Value = typename Handler::Type; + template + using Value = typename TypeHandler::Type; static constexpr int kSSOCapacity = 1; - using ElementFactory = void* (*)(Arena*); - protected: - // We use the same Handler for all Message types to deduplicate generated + // We use the same TypeHandler for all Message types to deduplicate generated // code. - template + template using CommonHandler = typename std::conditional< - std::is_base_of>::value, - internal::GenericTypeHandler, Handler>::type; + std::is_base_of>::value, + GenericTypeHandler, TypeHandler>::type; constexpr RepeatedPtrFieldBase() : tagged_rep_or_elem_(nullptr), @@ -145,7 +152,7 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { ~RepeatedPtrFieldBase() { #ifndef NDEBUG - // Try to trigger segfault / asan failure in non-opt builds. If arena_ + // Try to trigger segfault / asan failure in non-opt builds if arena_ // lifetime has ended before the destructor. if (arena_) (void)arena_->SpaceAllocated(); #endif @@ -182,14 +189,18 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { return cast(element_at(index)); } - template - Value* Add() { - if (std::is_same, std::string>{}) { - return cast(AddString()); - } - return cast(AddMessageLite(Handler::GetNewFunc())); + template + Value* Add() { + return cast(AddInternal(TypeHandler::GetNewFunc())); } + template + Value* AddFromPrototype(const Value* prototype) { + return cast( + AddInternal(TypeHandler::GetNewFromPrototypeFunc(prototype))); + } + + // TODO: Rework in terms of `AddInternal()`. template < typename TypeHandler, typename std::enable_if::type* = nullptr> @@ -247,12 +258,6 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { return *cast(element_at(index)); } - // Creates and adds an element using the given prototype, without introducing - // a link-time dependency on the concrete message type. - // - // Pre-condition: prototype must not be nullptr. - MessageLite* AddMessage(const MessageLite* prototype); - template void Clear() { const int n = current_size_; @@ -318,9 +323,9 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { template void CopyFrom(const RepeatedPtrFieldBase& other) { if (&other == this) return; - RepeatedPtrFieldBase::Clear(); + Clear(); if (other.empty()) return; - RepeatedPtrFieldBase::MergeFrom(other); + MergeFrom(other); } void CloseGap(int start, int num); @@ -355,12 +360,7 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { template PROTOBUF_NDEBUG_INLINE void Swap(RepeatedPtrFieldBase* other) { -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) -#else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP - { + if (internal::CanUseInternalSwap(GetArena(), other->GetArena())) { InternalSwap(other); } else { SwapFallback(other); @@ -512,11 +512,7 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { template PROTOBUF_NOINLINE void SwapFallback(RepeatedPtrFieldBase* other) { -#ifdef PROTOBUF_FORCE_COPY_IN_SWAP - ABSL_DCHECK(GetArena() == nullptr || other->GetArena() != GetArena()); -#else // PROTOBUF_FORCE_COPY_IN_SWAP - ABSL_DCHECK(other->GetArena() != GetArena()); -#endif // !PROTOBUF_FORCE_COPY_IN_SWAP + ABSL_DCHECK(!internal::CanUseInternalSwap(GetArena(), other->GetArena())); // Copy semantics in this case. We try to improve efficiency by placing the // temporary on |other|'s arena so that messages are copied twice rather @@ -726,10 +722,6 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { return InternalExtend(n - Capacity()); } - // Internal helpers for Add that keep definition out-of-line. - void* AddMessageLite(ElementFactory factory); - void* AddString(); - // Common implementation used by various Add* methods. `factory` is an object // used to construct a new element unless there are spare cleared elements // ready for reuse. Returns pointer to the new element. @@ -776,8 +768,8 @@ void RepeatedPtrFieldBase::MergeFrom( const RepeatedPtrFieldBase& from); -template -void* RepeatedPtrFieldBase::AddInternal(F factory) { +template +void* RepeatedPtrFieldBase::AddInternal(Factory factory) { Arena* const arena = GetArena(); if (tagged_rep_or_elem_ == nullptr) { ExchangeCurrentSize(1); @@ -817,27 +809,45 @@ PROTOBUF_EXPORT void InternalOutOfLineDeleteMessageLite(MessageLite* message); template class GenericTypeHandler { public: - typedef GenericType Type; - using Movable = IsMovable; + using Type = GenericType; + using Movable = IsMovable; static constexpr auto GetNewFunc() { return Arena::DefaultConstruct; } + static constexpr auto GetNewFromPrototypeFunc(const Type* prototype) { + return [prototype](Arena* arena) { + ABSL_DCHECK(prototype != nullptr); + return prototype->New(arena); + }; + } + static inline Arena* GetArena(Type* value) { + return Arena::InternalGetArena(value); + } - static inline GenericType* New(Arena* arena) { - return static_cast(Arena::DefaultConstruct(arena)); + static inline Type* New(Arena* arena) { + return static_cast(Arena::DefaultConstruct(arena)); } - static inline GenericType* New(Arena* arena, GenericType&& value) { - return Arena::Create(arena, std::move(value)); + static inline Type* New(Arena* arena, Type&& value) { + return Arena::Create(arena, std::move(value)); } - static inline GenericType* NewFromPrototype(const GenericType* /*prototype*/, - Arena* arena = nullptr) { - return New(arena); + static inline Type* NewFromPrototype(const Type* prototype, + Arena* arena = nullptr) { +#if __cpp_if_constexpr + if constexpr (std::is_base_of::value) { +#else + if (std::is_base_of::value) { +#endif + ABSL_DCHECK(prototype != nullptr); + return prototype->New(arena); + } else { + return New(arena); + } } - static inline void Delete(GenericType* value, Arena* arena) { + static inline void Delete(Type* value, Arena* arena) { if (arena != nullptr) return; #ifdef __cpp_if_constexpr - if constexpr (std::is_base_of::value) { + if constexpr (std::is_base_of::value) { // Using virtual destructor to reduce generated code size that would have - // happened otherwise due to inlined `~GenericType`. + // happened otherwise due to inlined `~Type()`. InternalOutOfLineDeleteMessageLite(value); } else { delete value; @@ -846,86 +856,56 @@ class GenericTypeHandler { delete value; #endif } - static inline Arena* GetArena(GenericType* value) { - return Arena::InternalGetArena(value); + static inline void Clear(Type* value) { value->Clear(); } + static inline void Merge(const Type& from, Type* to) { +#if __cpp_if_constexpr + if constexpr (std::is_base_of::value) { +#else + if (std::is_base_of::value) { +#endif + to->CheckTypeAndMergeFrom(from); + } else { + to->MergeFrom(from); + } } - - static inline void Clear(GenericType* value) { value->Clear(); } - static void Merge(const GenericType& from, GenericType* to); - static inline size_t SpaceUsedLong(const GenericType& value) { + static inline size_t SpaceUsedLong(const Type& value) { return value.SpaceUsedLong(); } }; -// NewFromPrototypeHelper() is not defined inline here, as we will need to do a -// virtual function dispatch anyways to go from Message* to call New/Merge. (The -// additional helper is needed as a workaround for MSVC.) -PROTOBUF_EXPORT MessageLite* NewFromPrototypeHelper( - const MessageLite* prototype, Arena* arena); - -template <> -inline MessageLite* GenericTypeHandler::NewFromPrototype( - const MessageLite* prototype, Arena* arena) { - return NewFromPrototypeHelper(prototype, arena); -} -template <> -inline Arena* GenericTypeHandler::GetArena(MessageLite* value) { - return value->GetArena(); -} - -template -PROTOBUF_NOINLINE inline void GenericTypeHandler::Merge( - const GenericType& from, GenericType* to) { - to->MergeFrom(from); -} -template <> -PROTOBUF_EXPORT void GenericTypeHandler::Merge( - const MessageLite& from, MessageLite* to); - -// Message specialization bodies defined in message.cc. This split is necessary -// to allow proto2-lite (which includes this header) to be independent of -// Message. -template <> -PROTOBUF_EXPORT Message* GenericTypeHandler::NewFromPrototype( - const Message* prototype, Arena* arena); -template <> -PROTOBUF_EXPORT Arena* GenericTypeHandler::GetArena(Message* value); - -PROTOBUF_EXPORT void* NewStringElement(Arena* arena); - template <> class GenericTypeHandler { public: - typedef std::string Type; + using Type = std::string; using Movable = IsMovable; - static constexpr auto GetNewFunc() { return NewStringElement; } - - static PROTOBUF_NOINLINE std::string* New(Arena* arena) { - return Arena::Create(arena); + static constexpr auto* GetNewFunc() { return Arena::Create; } + static constexpr auto GetNewFromPrototypeFunc(const Type* prototype) { + return GetNewFunc(); } - static PROTOBUF_NOINLINE std::string* New(Arena* arena, std::string&& value) { - return Arena::Create(arena, std::move(value)); + static PROTOBUF_NOINLINE Type* New(Arena* arena) { + return Arena::Create(arena); } - static inline std::string* NewFromPrototype(const std::string*, - Arena* arena) { + static PROTOBUF_NOINLINE Type* New(Arena* arena, Type&& value) { + return Arena::Create(arena, std::move(value)); + } + static inline Type* NewFromPrototype(const Type*, Arena* arena) { return New(arena); } - static inline Arena* GetArena(std::string*) { return nullptr; } - static inline void Delete(std::string* value, Arena* arena) { + static inline void Delete(Type* value, Arena* arena) { if (arena == nullptr) { delete value; } } - static inline void Clear(std::string* value) { value->clear(); } - static inline void Merge(const std::string& from, std::string* to) { - *to = from; - } - static size_t SpaceUsedLong(const std::string& value) { + static inline Arena* GetArena(Type*) { return nullptr; } + static inline void Clear(Type* value) { value->clear(); } + static inline void Merge(const Type& from, Type* to) { *to = from; } + static inline size_t SpaceUsedLong(const Type& value) { return sizeof(value) + StringSpaceUsedExcludingSelfLong(value); } }; + } // namespace internal // RepeatedPtrField is like RepeatedField, but used for repeated strings or @@ -1009,6 +989,12 @@ class RepeatedPtrField final : private internal::RepeatedPtrFieldBase { // fastest API for adding a new element. pointer Add() ABSL_ATTRIBUTE_LIFETIME_BOUND; + // Like `Add()`, but creates a new element from a provided prototype. For + // strings, this is equivalent to `Add()`. For messages, this is equivalent + // to `AddAllocated(new T(prototype))`. + pointer AddFromPrototype(const_pointer prototype) + ABSL_ATTRIBUTE_LIFETIME_BOUND; + // `Add(std::move(value));` is equivalent to `*Add() = std::move(value);` // It will either move-construct to the end of this field, or swap value // with the new-or-recycled element at the end of this field. Note that @@ -1384,6 +1370,12 @@ inline Element* RepeatedPtrField::Add() ABSL_ATTRIBUTE_LIFETIME_BOUND { return RepeatedPtrFieldBase::Add(); } +template +Element* RepeatedPtrField::AddFromPrototype(const Element* prototype) + ABSL_ATTRIBUTE_LIFETIME_BOUND { + return RepeatedPtrFieldBase::AddFromPrototype(prototype); +} + template inline void RepeatedPtrField::Add(Element&& value) { RepeatedPtrFieldBase::Add(std::move(value));