-
Notifications
You must be signed in to change notification settings - Fork 282
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
src|python-bindings: Use std::vector<uint8_t>
for id
and memo
#302
src|python-bindings: Use std::vector<uint8_t>
for id
and memo
#302
Conversation
src/prover_disk.hpp
Outdated
void GetMemo(uint8_t* buffer) { memcpy(buffer, memo, this->memo_size); } | ||
|
||
uint32_t GetMemoSize() const noexcept { return this->memo_size; } | ||
std::vector<uint8_t> GetMemo() { return memo; } |
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.
would it make sense for this to return a span<const uint8_t>
or a const std::vector<uint8_t>&
? (as well as GetId()
below)
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 agree span
would be nice in many places but we have C++17 set here still so this feels out of scope. I changed the return to const reference though 👍 (https://github.com/Chia-Network/chiapos/compare/fb796346df762224973f92d4628b6e994b49ba00..b06302c6fc9f9c914110002698d91f3741cea6c6)
fb79634
to
b06302c
Compare
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 a clear improvement
python-bindings/chiapos.cpp
Outdated
delete[] memo; | ||
return ret; | ||
std::vector<uint8_t> memo = dp.GetMemo(); | ||
return py::bytes(reinterpret_cast<char*>(memo.data()), memo.size()); |
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.
Its probably not a big deal, but this will copy the buffer twice, where i would think once is enough
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.
Changed!
python-bindings/chiapos.cpp
Outdated
py::bytes ret = py::bytes(reinterpret_cast<char *>(id), kIdLen); | ||
delete[] id; | ||
return ret; | ||
std::vector<uint8_t> id = dp.GetId(); |
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 here
b06302c
to
2f8e259
Compare
No description provided.