Skip to content

Align generated code with new RepeatedPtrField paradigm. #23537

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Align generated code with new RepeatedPtrField paradigm. #23537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions src/google/protobuf/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -659,6 +659,7 @@ cc_library(
"has_bits.h",
"implicit_weak_message.h",
"inlined_string_field.h",
"internal_metadata_locator.h",
"map.h",
"map_field_lite.h",
"map_type_handler.h",
Expand Down Expand Up @@ -887,6 +888,25 @@ cc_test(
],
)

cc_test(
name = "internal_metadata_locator_test",
size = "small",
srcs = [
"internal_metadata_locator_test.cc",
],
deps = [
":arena",
":port",
":protobuf",
":protobuf_lite",
":test_util2",
":unittest_cc_proto",
"//src/google/protobuf/io",
"@googletest//:gtest",
"@googletest//:gtest_main",
],
)

# This provides just the header files for use in projects that need to build
# shared libraries for dynamic loading. This target is available until Bazel
# adds native support for such use cases.
Expand Down
21 changes: 17 additions & 4 deletions src/google/protobuf/arena.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,10 +446,19 @@ class PROTOBUF_EXPORT PROTOBUF_ALIGNAS(8)
Arena* PROTOBUF_NULLABLE arena,
Args&&... args) {
if constexpr (internal::IsRepeatedPtrFieldType<T>::value) {
using ArenaRepT = typename internal::RepeatedPtrFieldArenaRep<T>::Type;
auto* arena_repr =
new (ptr) ArenaRepT(arena, static_cast<Args&&>(args)...);
return arena_repr;
if (ABSL_PREDICT_FALSE(arena == nullptr)) {
return new (ptr) T(static_cast<Args&&>(args)...);
} else {
using ArenaRepT =
typename internal::RepeatedPtrFieldArenaRep<T>::Type;
auto* arena_repr =
new (ptr) ArenaRepT(arena, static_cast<Args&&>(args)...);
#ifdef PROTOBUF_INTERNAL_REMOVE_ARENA_PTRS_REPEATED_PTR_FIELD
return &arena_repr->field();
#else
return arena_repr;
#endif
}
} else {
return new (ptr) T(arena, static_cast<Args&&>(args)...);
}
Expand Down Expand Up @@ -559,7 +568,11 @@ class PROTOBUF_EXPORT PROTOBUF_ALIGNAS(8)
using ArenaRepT = typename internal::RepeatedPtrFieldArenaRep<T>::Type;
auto* arena_repr =
arena->DoCreateMessage<ArenaRepT>(static_cast<Args&&>(args)...);
#ifdef PROTOBUF_INTERNAL_REMOVE_ARENA_PTRS_REPEATED_PTR_FIELD
return &arena_repr->field();
#else
return arena_repr;
#endif
}
}

Expand Down
5 changes: 5 additions & 0 deletions src/google/protobuf/arena_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,12 @@ TEST(ArenaTest, RepeatedPtrFieldMoveCtorOnArena) {
TestUtil::ExpectAllFieldsSet(moved->Get(0));

// The only extra allocation with moves is sizeof(RepeatedPtrField).
#ifdef PROTOBUF_INTERNAL_REMOVE_ARENA_PTRS_REPEATED_PTR_FIELD
EXPECT_EQ(usage_by_move,
sizeof(internal::RepeatedPtrFieldWithArena<TestAllTypes>));
#else
EXPECT_EQ(usage_by_move, sizeof(internal::RepeatedPtrFieldBase));
#endif
EXPECT_LT(usage_by_move + sizeof(TestAllTypes), usage_original);

// Status after move is unspecified and must not be assumed. It's merely
Expand Down
49 changes: 47 additions & 2 deletions src/google/protobuf/compiler/cpp/field.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,20 @@ namespace google {
namespace protobuf {
namespace compiler {
namespace cpp {

namespace {
using ::google::protobuf::internal::WireFormat;
using Sub = ::google::protobuf::io::Printer::Sub;

void InternalMetadataOffsetFormatString(io::Printer* p) {
p->Emit(R"cc(
$pbi$::InternalMetadataOffset::Build<
$classtype$, offsetof($classtype$, _impl_.$name$_)>()
)cc");
}

} // namespace

std::vector<Sub> FieldVars(const FieldDescriptor* field, const Options& opts) {
bool split = ShouldSplit(field, opts);
std::vector<Sub> vars = {
Expand Down Expand Up @@ -156,7 +167,19 @@ void FieldGeneratorBase::GenerateMemberConstexprConstructor(
io::Printer* p) const {
ABSL_CHECK(!field_->is_extension());
if (field_->is_repeated()) {
p->Emit("$name$_{}");
if (IsRepeatedPtrField(field_)) {
p->Emit({{"internal_metadata_offset",
[p] { InternalMetadataOffsetFormatString(p); }}},
R"cc(
#ifdef PROTOBUF_INTERNAL_REMOVE_ARENA_PTRS_REPEATED_PTR_FIELD
$name$_{visibility, $internal_metadata_offset$}
#else // !PROTOBUF_INTERNAL_REMOVE_ARENA_PTRS_REPEATED_PTR_FIELD
$name$_ {}
#endif
)cc");
} else {
p->Emit("$name$_{}");
}
} else {
p->Emit({{"default", DefaultValue(options_, field_)}},
"$name$_{$default$}");
Expand All @@ -170,6 +193,16 @@ void FieldGeneratorBase::GenerateMemberConstructor(io::Printer* p) const {
} else if (field_->is_repeated()) {
if (ShouldSplit(field_, options_)) {
p->Emit("$name$_{}"); // RawPtr<Repeated>
} else if (IsRepeatedPtrField(field_)) {
p->Emit({{"internal_metadata_offset",
[p] { InternalMetadataOffsetFormatString(p); }}},
R"cc(
#ifdef PROTOBUF_INTERNAL_REMOVE_ARENA_PTRS_REPEATED_PTR_FIELD
$name$_{visibility, $internal_metadata_offset$}
#else
$name$_ { visibility, arena }
#endif
)cc");
} else {
p->Emit("$name$_{visibility, arena}");
}
Expand All @@ -182,7 +215,19 @@ void FieldGeneratorBase::GenerateMemberConstructor(io::Printer* p) const {
void FieldGeneratorBase::GenerateMemberCopyConstructor(io::Printer* p) const {
ABSL_CHECK(!field_->is_extension());
if (field_->is_repeated()) {
p->Emit("$name$_{visibility, arena, from.$name$_}");
if (IsRepeatedPtrField(field_)) {
p->Emit({{"internal_metadata_offset",
[p] { InternalMetadataOffsetFormatString(p); }}},
R"cc(
#ifdef PROTOBUF_INTERNAL_REMOVE_ARENA_PTRS_REPEATED_PTR_FIELD
$name$_{visibility, ($internal_metadata_offset$), from.$name$_}
#else
$name$_ { visibility, arena, from.$name$_ }
#endif
)cc");
} else {
p->Emit("$name$_{visibility, arena, from.$name$_}");
}
} else {
p->Emit("$name$_{from.$name$_}");
}
Expand Down
39 changes: 36 additions & 3 deletions src/google/protobuf/compiler/cpp/field_generators/message_field.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -740,6 +740,8 @@ class RepeatedMessage : public FieldGeneratorBase {
void GenerateIsInitialized(io::Printer* p) const override;
bool NeedsIsInitialized() const override;

bool RequiresArena(GeneratorFunction function) const override;

private:
const Options* opts_;
bool has_required_;
Expand All @@ -748,7 +750,11 @@ class RepeatedMessage : public FieldGeneratorBase {
void RepeatedMessage::GeneratePrivateMembers(io::Printer* p) const {
if (should_split()) {
p->Emit(R"cc(
#ifdef PROTOBUF_INTERNAL_REMOVE_ARENA_PTRS_REPEATED_PTR_FIELD
$pbi$::RawPtr<$pbi$::$Weak$RepeatedPtrFieldWithArena<$Submsg$>> $name$_;
#else
$pbi$::RawPtr<$pb$::$Weak$RepeatedPtrField<$Submsg$>> $name$_;
#endif
)cc");
} else {
p->Emit("$pb$::$Weak$RepeatedPtrField< $Submsg$ > $name$_;\n");
Expand Down Expand Up @@ -831,7 +837,8 @@ void RepeatedMessage::GenerateInlineAccessorDefinitions(io::Printer* p) const {
ABSL_ATTRIBUTE_LIFETIME_BOUND {
$WeakDescriptorSelfPin$;
$TsanDetectConcurrentMutation$;
$Submsg$* _add = _internal_mutable_$name_internal$()->Add();
$Submsg$* _add = _internal_mutable_$name_internal$()->AddWithArena(
$pb$::MessageLite::internal_visibility(), GetArena());
$set_hasbit$;
$annotate_add_mutable$;
// @@protoc_insertion_point(field_add:$pkg.Msg.field$)
Expand All @@ -854,17 +861,31 @@ void RepeatedMessage::GenerateInlineAccessorDefinitions(io::Printer* p) const {
inline const $pb$::$Weak$RepeatedPtrField<$Submsg$>&
$Msg$::_internal$_weak$_$name_internal$() const {
$TsanDetectConcurrentRead$;
#ifdef PROTOBUF_INTERNAL_REMOVE_ARENA_PTRS_REPEATED_PTR_FIELD
return $field_$->field();
#else
return *$field_$;
#endif
}
inline $pb$::$Weak$RepeatedPtrField<$Submsg$>* $nonnull$
$Msg$::_internal_mutable$_weak$_$name_internal$() {
$TsanDetectConcurrentRead$;
$PrepareSplitMessageForWrite$;
if ($field_$.IsDefault()) {
#ifdef PROTOBUF_INTERNAL_REMOVE_ARENA_PTRS_REPEATED_PTR_FIELD
$field_$.Set($superclass$::DefaultConstruct<
$pbi$::$Weak$RepeatedPtrFieldWithArena<$Submsg$>>(
GetArena()));
#else
$field_$.Set($superclass$::DefaultConstruct<
$pb$::$Weak$RepeatedPtrField<$Submsg$>>(GetArena()));
#endif
}
#ifdef PROTOBUF_INTERNAL_REMOVE_ARENA_PTRS_REPEATED_PTR_FIELD
return &$field_$->field();
#else
return $field_$.Get();
#endif
}
)cc");
} else {
Expand Down Expand Up @@ -908,7 +929,8 @@ void RepeatedMessage::GenerateMergingCode(io::Printer* p) const {
// `if (!from.empty()) { body(); }` for both split and non-split cases.
auto body = [&] {
p->Emit(R"cc(
_this->_internal_mutable$_weak$_$name$()->MergeFrom(
_this->_internal_mutable$_weak$_$name$()->MergeFromWithArena(
$pb$::MessageLite::internal_visibility(), arena,
from._internal$_weak$_$name$());
)cc");
};
Expand Down Expand Up @@ -940,7 +962,9 @@ void RepeatedMessage::GenerateCopyConstructorCode(io::Printer* p) const {
if (should_split()) {
p->Emit(R"cc(
if (!from._internal$_weak$_$name$().empty()) {
_internal_mutable$_weak$_$name$()->MergeFrom(from._internal$_weak$_$name$());
_internal_mutable$_weak$_$name$()->MergeFromWithArena(
$pb$::MessageLite::internal_visibility(), arena,
from._internal$_weak$_$name$());
}
)cc");
}
Expand Down Expand Up @@ -1047,6 +1071,15 @@ void RepeatedMessage::GenerateIsInitialized(io::Printer* p) const {
}

bool RepeatedMessage::NeedsIsInitialized() const { return has_required_; }

bool RepeatedMessage::RequiresArena(GeneratorFunction func) const {
switch (func) {
case GeneratorFunction::kMergeFrom:
return true;
}
return false;
}

} // namespace

std::unique_ptr<FieldGeneratorBase> MakeSinguarMessageGenerator(
Expand Down
26 changes: 20 additions & 6 deletions src/google/protobuf/compiler/cpp/field_generators/string_field.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,12 +753,22 @@ class RepeatedString : public FieldGeneratorBase {
}
}

bool RequiresArena(GeneratorFunction function) const override {
switch (function) {
case GeneratorFunction::kMergeFrom:
return true;
}
return false;
}

void GenerateMergingCode(io::Printer* p) const override {
// TODO: experiment with simplifying this to be
// `if (!from.empty()) { body(); }` for both split and non-split cases.
auto body = [&] {
p->Emit(R"cc(
_this->_internal_mutable_$name$()->MergeFrom(from._internal_$name$());
_this->_internal_mutable_$name$()->MergeFromWithArena(
$pb$::MessageLite::internal_visibility(), arena,
from._internal_$name$());
)cc");
};
if (!should_split()) {
Expand Down Expand Up @@ -793,7 +803,9 @@ class RepeatedString : public FieldGeneratorBase {
if (should_split()) {
p->Emit(R"cc(
if (!from._internal_$name$().empty()) {
_internal_mutable_$name$()->MergeFrom(from._internal_$name$());
_internal_mutable_$name$()->MergeFromWithArena(
$pb$::MessageLite::internal_visibility(), arena,
from._internal_$name$());
}
)cc");
}
Expand Down Expand Up @@ -869,7 +881,8 @@ void RepeatedString::GenerateInlineAccessorDefinitions(io::Printer* p) const {
ABSL_ATTRIBUTE_LIFETIME_BOUND {
$WeakDescriptorSelfPin$;
$TsanDetectConcurrentMutation$;
::std::string* _s = _internal_mutable_$name_internal$()->Add();
::std::string* _s = _internal_mutable_$name_internal$()->AddWithArena(
$pb$::MessageLite::internal_visibility(), GetArena());
$set_hasbit$;
$annotate_add_mutable$;
// @@protoc_insertion_point(field_add_mutable:$pkg.Msg.field$)
Expand Down Expand Up @@ -907,9 +920,10 @@ void RepeatedString::GenerateInlineAccessorDefinitions(io::Printer* p) const {
inline void $Msg$::add_$name$(Arg_&& value, Args_... args) {
$WeakDescriptorSelfPin$;
$TsanDetectConcurrentMutation$;
$pbi$::AddToRepeatedPtrField(*_internal_mutable_$name_internal$(),
::std::forward<Arg_>(value),
args... $bytes_tag$);
$pbi$::AddToRepeatedPtrField(
$pb$::MessageLite::internal_visibility(), GetArena(),
*_internal_mutable_$name_internal$(), ::std::forward<Arg_>(value),
args... $bytes_tag$);
$set_hasbit$;
$annotate_add$;
// @@protoc_insertion_point(field_add:$pkg.Msg.field$)
Expand Down
20 changes: 17 additions & 3 deletions src/google/protobuf/compiler/cpp/field_generators/string_view_field.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,12 +630,22 @@ class RepeatedStringView : public FieldGeneratorBase {
}
}

bool RequiresArena(GeneratorFunction function) const override {
switch (function) {
case GeneratorFunction::kMergeFrom:
return true;
}
return false;
}

void GenerateMergingCode(io::Printer* p) const override {
// TODO: experiment with simplifying this to be
// `if (!from.empty()) { body(); }` for both split and non-split cases.
auto body = [&] {
p->Emit(R"cc(
_this->_internal_mutable_$name$()->MergeFrom(from._internal_$name$());
_this->_internal_mutable_$name$()->MergeFromWithArena(
$pb$::MessageLite::internal_visibility(), arena,
from._internal_$name$());
)cc");
};
if (!should_split()) {
Expand Down Expand Up @@ -670,7 +680,9 @@ class RepeatedStringView : public FieldGeneratorBase {
if (should_split()) {
p->Emit(R"cc(
if (!from._internal_$name$().empty()) {
_internal_mutable_$name$()->MergeFrom(from._internal_$name$());
_internal_mutable_$name$()->MergeFromWithArena(
$pb$::MessageLite::internal_visibility(), arena,
from._internal_$name$());
}
)cc");
}
Expand Down Expand Up @@ -757,7 +769,9 @@ void RepeatedStringView::GenerateInlineAccessorDefinitions(
inline void $Msg$::add_$name$(Arg_&& value) {
$WeakDescriptorSelfPin$;
$TsanDetectConcurrentMutation$;
$pbi$::AddToRepeatedPtrField(*_internal_mutable_$name_internal$(),
$pbi$::AddToRepeatedPtrField($pb$::MessageLite::internal_visibility(),
GetArena(),
*_internal_mutable_$name_internal$(),
::std::forward<Arg_>(value) $bytes_tag$);
$set_hasbit$;
$annotate_add$;
Expand Down
Loading
Loading