Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add functionality so Starcoder can send back arbitrary PMTs, not just blobs #78
Add functionality so Starcoder can send back arbitrary PMTs, not just blobs #78
Changes from 23 commits
93c33b5
68e50c0
3b353b5
c7b6653
044f5ad
c873e2d
e62c48d
dc1308a
2efc6e4
359b1c1
f8c4b65
2c33d2e
a610968
dc5fc29
044768e
e4fbdbc
bf3e2ee
c6e7ab7
ad27a4d
db77770
df2b028
5686c67
1215f26
8c1522d
26a5a50
55f1eea
d9d22b9
3e95525
c2b222e
d0df957
a9b4413
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
My C++ knowledge hasn't been update for C++11 and I really don't know how initializer expressions work... Can you explain how this assignment works? My reasoning is the right hand side calls
RepeatedPtrField::RepeatedPtrField(Iter begin, const Iter & end)
and then the whole content is copied for the another time, into the left hand side, which is redundant. You usedtransform
and an inserter below. Why this piece of code follows a different pattern?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 changed it to use
std::copy
. Wdyt?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.
Looks good 👍
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.
Don't you need to use a back inserter? Is it guaranteed that
c32_vector
has enough buffer to store all transformed elements?https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.repeated_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.
Okay, I modified it. Could you check if I did it correctly? Sorry, I don't fully understand STL containers yet...
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.
STL algorithms write to output iterators. And a vector's iterator is just the address of underlying memory buffer. So outputting to an iterator is no more than writing to a memory buffer. It won't change the container size, it won't reallocate the memory buffer if there is enough room.
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.
same as above
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.
Feel free to pass
BlockMessage
back as a return value. I believe the compiler is smart enough to avoid copying an object when returning it (and actually this behavior is required by the language specification.)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.
This is what I did first. The problem is the C++ generated protobuf has no way to set embedded message fields other than through getting a pointer to the embedded message field with the
mutable_xxx()
method and directly modifying that pointer (there is noset_xxx()
method). See https://stackoverflow.com/questions/43268845/do-i-need-to-delete-objects-passed-to-google-protocol-buffer-protobuf. For the recursion to work, not sure if there's another way than having the function take a pointer.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.
It seems you can call
Swap()
on a mutable message. I haven't verified this API on my side and leave the decision up to you.https://developers.google.com/protocol-buffers/docs/reference/cpp-generated#message
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 guess this would work. Is there a specific reason to prefer returning the
BlockMessage
?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.
This is more or less a matter of personal preference so I left it up to you. But do you think it is more straightforward to return a value via a pointer argument than to use a return value? You wouldn't do it if you are programming in another language.
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.
Just for reference, found these about the protobuf move semantics. Not sure if copy when reallocating affects us
protocolbuffers/protobuf#2791
protocolbuffers/protobuf#3630
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.
FYI. No explicit move is needed in this case. Its optimization for returning a local variable from a function.
https://gist.github.com/kenichi-fukushima/197238e0cd23b82666a27abfa5fbd540
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.
@kenichi-fukushima Converted this to your suggested function signature. It does seem more intuitive to use.
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.
It seems like this function is only used by
convert_pmt_to_proto
and does not need to be exposes in a header file.