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 1 commit
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
32 changes: 32 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,
const std::string&& value) const {
Copy link

Choose a reason for hiding this comment

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

@reed-lau rvalue references cannot be const as they cannot be moved. The overloaded functions rvalue parameter should be SetString(..,..,std::string&& value) without the const!

USAGE_CHECK_ALL(SetString, SINGULAR, STRING);
if (field->is_extension()) {
return MutableExtensionSet(message)->SetString(field->number(),
field->type(), value, field);
Copy link

Choose a reason for hiding this comment

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

Also here i believe the value parameter (i.e. the input string) should be moved, so:
SetString(...,..., std::move(value), field)
Additionaly, the string which you are creating in your code should also be moved into place:

bool foo(const char *buf, size_t size,...) { ... std::string temp(buf, size); // actually, we don't need to create a temporary std::string here, if we have // some method like SetStringByArray(..., buf, size); reflection->SetString(...., std::move(temp)); ... }

} 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))) = value;
break;
}
}
}
}


std::string Reflection::GetRepeatedString(const Message& message,
const FieldDescriptor* field,
int index) const {
Expand Down
2 changes: 2 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,
const 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