-
Notifications
You must be signed in to change notification settings - Fork 4k
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
THRIFT-5816: Fix UUID for boost 1.86.0 (change in {{data}} member usage) #3035
THRIFT-5816: Fix UUID for boost 1.86.0 (change in {{data}} member usage) #3035
Conversation
- Not sure why I am seeing this only now
- Appears to break in older compilers
- Accessing the data member directly is discouraged - Iterate through the uuid type instead
@Jens-G |
client: cpp Patch: Carel Combrink This closes #3035
@@ -112,8 +112,8 @@ class TestHandler : public ThriftTestIf { | |||
out = thing; | |||
} | |||
|
|||
std::string testUuid(const std::string thing) override { | |||
cout << "[C -> C++] testUuid(\"" << std::hex << thing << "\")" << '\n'; | |||
apache::thrift::TUuid testUuid(const apache::thrift::TUuid thing) override { |
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.
If you use const ref on the parameter and return value instead, it avoids copies.
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.
Unfortunately thirft generates the interface with a copy. Perhaps it is something I missed when I added the new type because I see the other functions uses const ref on some arguments
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.
@Jens-G Should I create a ticket for this and fix asap?
It would change how the method is generated if I change it to be a reference:
It appears to change to
virtual void testUuid(apache::thrift::TUuid& _return, const apache::thrift::TUuid& thing) = 0;
but I can't work on this more at this moment, probably only tonight again
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.
@@ -107,8 +107,8 @@ class TestHandler : public ThriftTestIf { | |||
out = thing; | |||
} | |||
|
|||
std::string testUuid(const std::string thing) override { | |||
cout << "[C -> C++] testUuid(\"" << std::hex << thing << "\")" << '\n'; | |||
apache::thrift::TUuid testUuid(const apache::thrift::TUuid thing) override { |
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.
If you use const ref on the parameter and return value instead, it avoids copies.
Tries to fix compiler issues with latest boost (1.86)
THRIFT-5816
[skip ci]
anywhere in the commit message to free up build resources.