Skip to content

Commit

Permalink
Improve MergeFrom behavior for oneof fields:
Browse files Browse the repository at this point in the history 
 - Avoid redundant calls to `GetArena()`.
 - Only do a single call to `xxx_clear()` if needed.
 - Set the oneof_case once, if needed.
 - Use CopyConstruct for new objects, like we do for non-oneof Merge.
 - Avoid the _Internal::mutable_xxx functions, as they are not needed anymore.

PiperOrigin-RevId: 585705944
  • Loading branch information
@protobuf-github-bot
protobuf-github-bot authored and Copybara-Service committed Nov 27, 2023
1 parent 7c5e18b commit 485404e
Show file tree
Hide file tree
Showing 7 changed files with 126 additions and 67 deletions.
26 changes: 20 additions & 6 deletions src/google/protobuf/compiler/cpp/field_generators/cord_field.cc
View file
Expand Up @@ -119,8 +119,10 @@ class CordOneofFieldGenerator : public CordFieldGenerator {
void GenerateInlineAccessorDefinitions(io::Printer* printer) const override;
void GenerateNonInlineAccessorDefinitions(
io::Printer* printer) const override;
bool RequiresArena(GeneratorFunction func) const override;
void GenerateClearingCode(io::Printer* printer) const override;
void GenerateSwappingCode(io::Printer* printer) const override;
void GenerateMergingCode(io::Printer* printer) const override;
void GenerateConstructorCode(io::Printer* printer) const override {}
void GenerateArenaDestructorCode(io::Printer* printer) const override;
// Overrides CordFieldGenerator behavior.
Expand Down Expand Up @@ -347,7 +349,7 @@ void CordOneofFieldGenerator::GenerateInlineAccessorDefinitions(
}
)cc");
printer->Emit(R"cc(
inline void $classname$::_internal_set_$name$(const ::absl::Cord& value) {
inline void $classname$::set_$name$(const ::absl::Cord& value) {
if ($not_has_field$) {
clear_$oneof_name$();
set_has_$name$();
Expand All @@ -358,11 +360,6 @@ void CordOneofFieldGenerator::GenerateInlineAccessorDefinitions(
}
}
*$field$ = value;
}
)cc");
printer->Emit(R"cc(
inline void $classname$::set_$name$(const ::absl::Cord& value) {
_internal_set_$name$(value);
$annotate_set$;
// @@protoc_insertion_point(field_set:$full_name$)
}
Expand Down Expand Up @@ -411,6 +408,14 @@ void CordOneofFieldGenerator::GenerateNonInlineAccessorDefinitions(
}
}

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

void CordOneofFieldGenerator::GenerateClearingCode(io::Printer* printer) const {
Formatter format(printer, variables_);
format(
Expand All @@ -429,6 +434,15 @@ void CordOneofFieldGenerator::GenerateArenaDestructorCode(
// default behavior here.
}

void CordOneofFieldGenerator::GenerateMergingCode(io::Printer* printer) const {
printer->Emit(R"cc(
if (oneof_needs_init) {
_this->$field$ = ::$proto_ns$::Arena::Create<absl::Cord>(arena);
}
*_this->$field$ = *from.$field$;
)cc");
}

// ===================================================================
} // namespace

Expand Down
17 changes: 7 additions & 10 deletions src/google/protobuf/compiler/cpp/field_generators/enum_field.cc
View file
Expand Up @@ -74,7 +74,7 @@ class SingularEnum : public FieldGeneratorBase {

void GenerateMergingCode(io::Printer* p) const override {
p->Emit(R"cc(
_this->_internal_set_$name$(from._internal_$name$());
_this->$field_$ = from.$field_$;
)cc");
}

Expand Down Expand Up @@ -175,7 +175,12 @@ void SingularEnum::GenerateInlineAccessorDefinitions(io::Printer* p) const {
p->Emit(R"cc(
inline void $Msg$::set_$name$($Enum$ value) {
$PrepareSplitMessageForWrite$;
_internal_set_$name$(value);
$assert_valid$;
if ($not_has_field$) {
clear_$oneof_name$();
set_has_$name$();
}
$field_$ = value;
$annotate_set$;
// @@protoc_insertion_point(field_set:$pkg.Msg.field$)
}
Expand All @@ -185,14 +190,6 @@ void SingularEnum::GenerateInlineAccessorDefinitions(io::Printer* p) const {
}
return static_cast<$Enum$>($kDefault$);
}
inline void $Msg$::_internal_set_$name$($Enum$ value) {
$assert_valid$;
if ($not_has_field$) {
clear_$oneof_name$();
set_has_$name$();
}
$field_$ = value;
}
)cc");
} else {
p->Emit(R"cc(
Expand Down
55 changes: 34 additions & 21 deletions src/google/protobuf/compiler/cpp/field_generators/message_field.cc
View file
Expand Up @@ -322,7 +322,7 @@ void SingularMessage::GenerateInternalAccessorDefinitions(
// practice, the linker is then not able to throw them out making implicit
// weak dependencies not work at all.

if (!is_weak()) return;
if (!is_weak() || is_oneof()) return;

// These private accessors are used by MergeFrom and
// MergePartialFromCodedStream, and their purpose is to provide access to
Expand All @@ -336,28 +336,11 @@ void SingularMessage::GenerateInternalAccessorDefinitions(
msg->$set_hasbit$;
)cc");
}},
{"is_already_set",
[&] {
if (!is_oneof()) {
p->Emit("msg->$field_$ == nullptr");
} else {
p->Emit("msg->$not_has_field$");
}
}},
{"clear_oneof",
[&] {
if (!is_oneof()) return;
p->Emit(R"cc(
msg->clear_$oneof_name$();
msg->set_has_$name$();
)cc");
}},
},
R"cc(
$pb$::MessageLite* $Msg$::_Internal::mutable_$name$($Msg$* msg) {
$update_hasbit$;
if ($is_already_set$) {
$clear_oneof$;
if (msg->$field_$ == nullptr) {
msg->$field_$ = $kDefaultPtr$->New(msg->GetArena());
}
return msg->$field_$;
Expand Down Expand Up @@ -398,7 +381,7 @@ void SingularMessage::GenerateMessageClearingCode(io::Printer* p) const {
bool SingularMessage::RequiresArena(GeneratorFunction function) const {
switch (function) {
case GeneratorFunction::kMergeFrom:
return !(is_weak() || is_oneof() || should_split());
return !(is_weak() || should_split());
}
return false;
}
Expand All @@ -408,7 +391,7 @@ void SingularMessage::GenerateMergingCode(io::Printer* p) const {
p->Emit(
"_Internal::mutable_$name$(_this)->CheckTypeAndMergeFrom(\n"
" *from.$field_$);\n");
} else if (is_oneof() || should_split()) {
} else if (should_split()) {
p->Emit(
"_this->_internal_mutable_$name$()->$Submsg$::MergeFrom(\n"
" from._internal_$name$());\n");
Expand Down Expand Up @@ -548,6 +531,8 @@ class OneofMessage : public SingularMessage {
void GenerateDestructorCode(io::Printer* p) const override;
void GenerateConstructorCode(io::Printer* p) const override;
void GenerateIsInitialized(io::Printer* p) const override;
void GenerateMergingCode(io::Printer* p) const override;
bool RequiresArena(GeneratorFunction func) const override;
};

void OneofMessage::GenerateNonInlineAccessorDefinitions(io::Printer* p) const {
Expand Down Expand Up @@ -689,6 +674,34 @@ void OneofMessage::GenerateIsInitialized(io::Printer* p) const {
)cc");
}

void OneofMessage::GenerateMergingCode(io::Printer* p) const {
if (is_weak()) {
p->Emit(R"cc(
if (oneof_needs_init) {
_this->$field_$ = from.$field_$->New(arena);
}
_this->$field_$->CheckTypeAndMergeFrom(*from.$field_$);
)cc");
} else {
p->Emit(R"cc(
if (oneof_needs_init) {
_this->$field_$ =
$superclass$::CopyConstruct<$Submsg$>(arena, *from.$field_$);
} else {
_this->$field_$->MergeFrom(from._internal_$name$());
}
)cc");
}
}

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

class RepeatedMessage : public FieldGeneratorBase {
public:
RepeatedMessage(const FieldDescriptor* field, const Options& opts,
Expand Down
15 changes: 6 additions & 9 deletions src/google/protobuf/compiler/cpp/field_generators/primitive_field.cc
View file
Expand Up @@ -108,7 +108,7 @@ class SingularPrimitive final : public FieldGeneratorBase {

void GenerateMergingCode(io::Printer* p) const override {
p->Emit(R"cc(
_this->_internal_set_$name$(from._internal_$name$());
_this->$field_$ = from.$field_$;
)cc");
}

Expand Down Expand Up @@ -198,7 +198,11 @@ void SingularPrimitive::GenerateInlineAccessorDefinitions(
p->Emit(R"cc(
inline void $Msg$::set_$name$($Type$ value) {
$PrepareSplitMessageForWrite$;
_internal_set_$name$(value);
if ($not_has_field$) {
clear_$oneof_name$();
set_has_$name$();
}
$field_$ = value;
$annotate_set$;
// @@protoc_insertion_point(field_set:$pkg.Msg.field$)
}
Expand All @@ -208,13 +212,6 @@ void SingularPrimitive::GenerateInlineAccessorDefinitions(
}
return $kDefault$;
}
inline void $Msg$::_internal_set_$name$($Type$ value) {
if ($not_has_field$) {
clear_$oneof_name$();
set_has_$name$();
}
$field_$ = value;
}
)cc");
} else {
p->Emit(R"cc(
Expand Down
23 changes: 20 additions & 3 deletions src/google/protobuf/compiler/cpp/field_generators/string_field.cc
View file
Expand Up @@ -88,10 +88,27 @@ class SingularString : public FieldGeneratorBase {
)cc");
}

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

void GenerateMergingCode(io::Printer* p) const override {
p->Emit(R"cc(
_this->_internal_set_$name$(from._internal_$name$());
)cc");
if (is_oneof()) {
p->Emit(R"cc(
if (oneof_needs_init) {
_this->$field_$.InitDefault();
}
_this->$field_$.Set(from._internal_$name$(), arena);
)cc");
} else {
p->Emit(R"cc(
_this->_internal_set_$name$(from._internal_$name$());
)cc");
}
}

void GenerateArenaDestructorCode(io::Printer* p) const override {
Expand Down
55 changes: 38 additions & 17 deletions src/google/protobuf/compiler/cpp/message.cc
View file
Expand Up @@ -3771,23 +3771,44 @@ void MessageGenerator::GenerateClassSpecificMergeImpl(io::Printer* p) {

// Merge oneof fields. Oneof field requires oneof case check.
for (auto oneof : OneOfRange(descriptor_)) {
format("switch (from.$1$_case()) {\n", oneof->name());
format.Indent();
for (auto field : FieldRange(oneof)) {
format("case k$1$: {\n", UnderscoresToCamelCase(field->name(), true));
format.Indent();
field_generators_.get(field).GenerateMergingCode(p);
format("break;\n");
format.Outdent();
format("}\n");
}
format(
"case $1$_NOT_SET: {\n"
" break;\n"
"}\n",
absl::AsciiStrToUpper(oneof->name()));
format.Outdent();
format("}\n");
p->Emit({{"name", oneof->name()},
{"NAME", absl::AsciiStrToUpper(oneof->name())},
{"index", oneof->index()},
{"cases",
[&] {
for (const auto* field : FieldRange(oneof)) {
p->Emit(
{{"Label", UnderscoresToCamelCase(field->name(), true)},
{"body",
[&] {
field_generators_.get(field).GenerateMergingCode(p);
}}},
R"cc(
case k$Label$: {
$body$;
break;
}
)cc");
}
}}},
R"cc(
if (const uint32_t oneof_from_case = from.$oneof_case$[$index$]) {
const uint32_t oneof_to_case = _this->$oneof_case$[$index$];
const bool oneof_needs_init = oneof_to_case != oneof_from_case;
if (oneof_needs_init) {
if (oneof_to_case != 0) {
_this->clear_$name$();
}
_this->$oneof_case$[$index$] = oneof_from_case;
}
switch (oneof_from_case) {
$cases$;
case $NAME$_NOT_SET:
break;
}
}
)cc");
}
if (num_weak_fields_) {
format(
Expand Down
2 changes: 1 addition & 1 deletion src/google/protobuf/cpp_features.pb.cc
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 485404e

Please sign in to comment.