-
Notifications
You must be signed in to change notification settings - Fork 15.6k
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.
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
C++ Add move constructor for Reflection's SetString #6477
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rvalue reference cannot be const
@@ -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 { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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)); ... }
@t-rybarski have fixed it according to your comment. |
@acozzette @protobuf-kokoro |
That Go error looks unrelated, so I wouldn't worry about it for now. |
src/google/protobuf/extension_set.h
Outdated
@@ -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); |
There was a problem hiding this comment.
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.
after change the interface to
|
This looks great but we are having some issues with Docker in our CI setup. Hopefully we will get the test issues sorted out soon and then I plan to merge this. |
Add move constructor for Reflection's SetString
add feature #6399