Skip to content
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

Jump to bottom

C++ Add move constructor for Reflection's SetString #6477

Merged
merged 4 commits into from Aug 9, 2019
Merged

C++ Add move constructor for Reflection's SetString #6477

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9f57cc9
Add move constructor for Reflection's SetString
reed-lau Aug 5, 2019
a42322f
add movable String interface Reflection
reed-lau Aug 5, 2019
47e4696
use move method for SetString
reed-lau Aug 5, 2019
839a7ae
replace 'const std::string&' with 'std::string'(by value)
reed-lau Aug 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions python/google/protobuf/pyext/message.cc
View file
Open in desktop
Expand Up @@ -788,11 +788,11 @@ bool CheckAndSetString(

string value_string(value, value_len);
if (append) {
reflection->AddString(message, descriptor, value_string);
reflection->AddString(message, descriptor, std::move(value_string));
} else if (index < 0) {
reflection->SetString(message, descriptor, value_string);
reflection->SetString(message, descriptor, std::move(value_string));
} else {
reflection->SetRepeatedString(message, descriptor, index, value_string);
reflection->SetRepeatedString(message, descriptor, index, std::move(value_string));
}
return true;
}
Expand Down
18 changes: 17 additions & 1 deletion src/google/protobuf/extension_set.h
View file
Open in desktop
Expand Up @@ -269,6 +269,7 @@ class PROTOBUF_EXPORT ExtensionSet {
void SetBool(int number, FieldType type, bool value, desc);
void SetEnum(int number, FieldType type, int value, desc);
void SetString(int number, FieldType type, const std::string& value, desc);
void SetString(int number, FieldType type, std::string&& value, desc);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it's possible to enable this optimization without creating new overloads. Could you try taking the existing methods and changing them to take a std::string (by value) instead of const std::string&? I think that should be backward-compatible with existing callers and still allow us to optimize the implementation.

std::string* MutableString(int number, FieldType type, desc);
MessageLite* MutableMessage(int number, FieldType type,
const MessageLite& prototype, desc);
Expand Down Expand Up @@ -332,6 +333,7 @@ class PROTOBUF_EXPORT ExtensionSet {
void SetRepeatedBool(int number, int index, bool value);
void SetRepeatedEnum(int number, int index, int value);
void SetRepeatedString(int number, int index, const std::string& value);
void SetRepeatedString(int number, int index, std::string&& value);
std::string* MutableRepeatedString(int number, int index);
MessageLite* MutableRepeatedMessage(int number, int index);

Expand All @@ -345,6 +347,7 @@ class PROTOBUF_EXPORT ExtensionSet {
void AddBool(int number, FieldType type, bool packed, bool value, desc);
void AddEnum(int number, FieldType type, bool packed, int value, desc);
void AddString(int number, FieldType type, const std::string& value, desc);
void AddString(int number, FieldType type, std::string&& value, desc);
std::string* AddString(int number, FieldType type, desc);
MessageLite* AddMessage(int number, FieldType type,
const MessageLite& prototype, desc);
Expand Down Expand Up @@ -862,16 +865,29 @@ inline void ExtensionSet::SetString(int number, FieldType type,
const FieldDescriptor* descriptor) {
MutableString(number, type, descriptor)->assign(value);
}
inline void ExtensionSet::SetString(int number, FieldType type,
std::string&& value,
const FieldDescriptor* descriptor) {
*MutableString(number, type, descriptor) = std::move(value);
}
inline void ExtensionSet::SetRepeatedString(int number, int index,
const std::string& value) {
MutableRepeatedString(number, index)->assign(value);
}
inline void ExtensionSet::SetRepeatedString(int number, int index,
std::string&& value) {
*MutableRepeatedString(number, index) = std::move(value);
}
inline void ExtensionSet::AddString(int number, FieldType type,
const std::string& value,
const FieldDescriptor* descriptor) {
AddString(number, type, descriptor)->assign(value);
}

inline void ExtensionSet::AddString(int number, FieldType type,
std::string&& value,
const FieldDescriptor* descriptor) {
*AddString(number, type, descriptor) = std::move(value);
}
// ===================================================================
// Glue for generated extension accessors

Expand Down
65 changes: 65 additions & 0 deletions src/google/protobuf/generated_message_reflection.cc
View file
Open in desktop
Expand Up @@ -1223,6 +1223,38 @@ void Reflection::SetString(Message* message, const FieldDescriptor* field,
}


void Reflection::SetString(Message* message, const FieldDescriptor* field,
std::string&& value) const {
USAGE_CHECK_ALL(SetString, SINGULAR, STRING);
if (field->is_extension()) {
return MutableExtensionSet(message)->SetString(field->number(),
field->type(), std::move(value), field);
} else {
switch (field->options().ctype()) {
default: // TODO(kenton): Support other string reps.
case FieldOptions::STRING: {
if (IsInlined(field)) {
MutableField<InlinedStringField>(message, field)
->SetNoArena(nullptr, value);
break;
}

const std::string* default_ptr =
&DefaultRaw<ArenaStringPtr>(field).Get();
if (field->containing_oneof() && !HasOneofField(*message, field)) {
ClearOneof(message, field->containing_oneof());
MutableField<ArenaStringPtr>(message, field)
->UnsafeSetDefault(default_ptr);
}
*(MutableField<ArenaStringPtr>(message, field)
->Mutable(default_ptr, GetArena(message))) = std::move(value);
break;
}
}
}
}


std::string Reflection::GetRepeatedString(const Message& message,
const FieldDescriptor* field,
int index) const {
Expand Down Expand Up @@ -1272,6 +1304,23 @@ void Reflection::SetRepeatedString(Message* message,
}


void Reflection::SetRepeatedString(Message* message,
const FieldDescriptor* field, int index,
std::string&& value) const {
USAGE_CHECK_ALL(SetRepeatedString, REPEATED, STRING);
if (field->is_extension()) {
MutableExtensionSet(message)->SetRepeatedString(field->number(), index,
std::move(value));
} else {
switch (field->options().ctype()) {
default: // TODO(kenton): Support other string reps.
case FieldOptions::STRING:
*MutableRepeatedField<std::string>(message, field, index) = std::move(value);
break;
}
}
}

void Reflection::AddString(Message* message, const FieldDescriptor* field,
const std::string& value) const {
USAGE_CHECK_ALL(AddString, REPEATED, STRING);
Expand All @@ -1289,6 +1338,22 @@ void Reflection::AddString(Message* message, const FieldDescriptor* field,
}


void Reflection::AddString(Message* message, const FieldDescriptor* field,
std::string&& value) const {
USAGE_CHECK_ALL(AddString, REPEATED, STRING);
if (field->is_extension()) {
MutableExtensionSet(message)->AddString(field->number(), field->type(),
std::move(value), field);
} else {
switch (field->options().ctype()) {
default: // TODO(kenton): Support other string reps.
case FieldOptions::STRING:
*AddField<std::string>(message, field) = std::move(value);
break;
}
}
}

// -------------------------------------------------------------------

const EnumValueDescriptor* Reflection::GetEnum(
Expand Down
6 changes: 6 additions & 0 deletions src/google/protobuf/message.h
View file
Open in desktop
Expand Up @@ -540,6 +540,8 @@ class PROTOBUF_EXPORT Reflection final {
bool value) const;
void SetString(Message* message, const FieldDescriptor* field,
const std::string& value) const;
void SetString(Message* message, const FieldDescriptor* field,
std::string&& value) const;
void SetEnum(Message* message, const FieldDescriptor* field,
const EnumValueDescriptor* value) const;
// Set an enum field's value with an integer rather than EnumValueDescriptor.
Expand Down Expand Up @@ -640,6 +642,8 @@ class PROTOBUF_EXPORT Reflection final {
int index, bool value) const;
void SetRepeatedString(Message* message, const FieldDescriptor* field,
int index, const std::string& value) const;
void SetRepeatedString(Message* message, const FieldDescriptor* field,
int index, std::string&& value) const;
void SetRepeatedEnum(Message* message, const FieldDescriptor* field,
int index, const EnumValueDescriptor* value) const;
// Set an enum field's value with an integer rather than EnumValueDescriptor.
Expand Down Expand Up @@ -677,6 +681,8 @@ class PROTOBUF_EXPORT Reflection final {
bool value) const;
void AddString(Message* message, const FieldDescriptor* field,
const std::string& value) const;
void AddString(Message* message, const FieldDescriptor* field,
std::string&& value) const;
void AddEnum(Message* message, const FieldDescriptor* field,
const EnumValueDescriptor* value) const;
// Add an integer value to a repeated enum field rather than
Expand Down