-
Notifications
You must be signed in to change notification settings - Fork 1
Implement struct serializer #32
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
base: master
Are you sure you want to change the base?
Conversation
|
Hi, Please wait for pr #31 to finish before you start writing on this pull request. |
|
Ok, #31 is merged. You could start to implement the class (make sure you have updated your local master branch). First change srs-control/test/backend/srs/UnitTestStruct.cpp Lines 83 to 87 in 8de00f5
Then, implement the convert function here to make sure the test succeeds. srs-control/backend/srs/converters/StructSerializer.cpp Lines 17 to 23 in 8de00f5
To do the testing, go to the ./unit_test_srs_backend |
a6009c8 to
cfb12f3
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.
Thanks for the PR. Here are some suggestions/comments for now:
| } | ||
|
|
||
| template<typename T> | ||
| constexpr auto bin_to_gray(T bin_val) |
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.
Maybe use binary_to_gray to be more consistent with gray_to_binary.
Both Serializer and Deserializer need access to bit fields MarkerDataCompact and HitDataCompact.
bin_to_gray -> binary_to_gray, avoided raw for loops in split_bits, formatting with clangd.format
0feb102 to
faac71a
Compare
This is actually much less readable. Maybe I will stick to the if conditins
Unit tests should have failed before
YanzhaoW
left a comment
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 great! Here are some further suggestions:
| auto output = std::vector<char>{}; | ||
| output.resize(sizeof(std::uint64_t) / common::BYTE_BIT_LENGTH); |
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.
Well, this is not a good decision to create a new vector in a loop. When a new vector is created, you have the memory allocation (in heap), which is very slow and should be avoided.
What you could do is to create some buffer vector and initialize it only once in the constructor. In the loop, before you write something into it, reset all its members to be zero. Your compact_to_vector should take an non-const ref as its output:
void compact (const T& compact_data, std::vector<char>& output);| auto output = std::vector<char>{}; | ||
| output.resize(sizeof(std::uint64_t) / common::BYTE_BIT_LENGTH); |
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.
You sure output.resize(sizeof(std::uint64_t) / common::BYTE_BIT_LENGTH); is correct? sizeof(std::uint64_t) / common::BYTE_BIT_LENGTH always results in 1.
No description provided.