Skip to content
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

Remove unnecessary special handling of large fields #532

Merged
merged 2 commits into from
Jan 4, 2021
Merged

Conversation

senier
Copy link
Member

@senier senier commented Dec 30, 2020

Close #531

I'm not sure why the special handling for unaligned fields / field larger than 7 bit even existed when using a bit array. All test still work, hence I assume removing that code should be OK. Please have a look.

@senier senier requested review from jklmnn and rssen December 30, 2020 18:46
Copy link
Contributor

@rssen rssen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no problem removing this.
We do already have a test test_message_value_odd_length_binary that is supposed to test the parsing of unaligned fields. Maybe this can be removed now, as your new test is more expressive.

jklmnn
jklmnn previously approved these changes Jan 4, 2021
@senier
Copy link
Member Author

senier commented Jan 4, 2021

I see no problem removing this.
We do already have a test test_message_value_odd_length_binary that is supposed to test the parsing of unaligned fields. Maybe this can be removed now, as your new test is more expressive.

Good catch - this test is not too useful anymore. I removed it in 7d572e9. Also rebased the branch to current develop.

@senier senier requested review from rssen and jklmnn January 4, 2021 11:49
@senier senier merged commit 5501894 into develop Jan 4, 2021
@senier senier deleted the issue_531 branch January 4, 2021 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fields crossing octet-boundary not serialized correctly
3 participants