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.

Sign up for GitHub

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

Jump to bottom

Fix bug in decoding multi-object MsgPack types #984

Merged
merged 12 commits into from
Aug 5, 2020
Merged

Fix bug in decoding multi-object MsgPack types #984

merged 12 commits into from
Aug 5, 2020

Conversation

mavam
Copy link
Member

@mavam mavam commented Jul 21, 2020

No description provided.

@mavam mavam requested a review from dominiklohmann July 21, 2020 09:19
@mavam mavam added the bug Incorrect behavior label Jul 21, 2020
dominiklohmann
dominiklohmann previously requested changes Jul 21, 2020
View reviewed changes
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

I've reverted the workarounds, added two assertions for divisibility without rest of size and spread, and added further unit tests that cover more type combinations.

It turns out that vector_type{port_type} is fixed, but vector_type{subnet_type} is still broken for MessagePack-encoded table slices. Nesting container types does not work for any of the table slice types, so I'm assuming this is a more general issue with the builders.

Please take another look at nesting subnets into vectors @mavam.

@dominiklohmann dominiklohmann force-pushed the story/ch18270 branch 3 times, most recently from 4d089b8 to 38b4c5a Compare August 5, 2020 13:06
@dominiklohmann dominiklohmann force-pushed the story/ch18270 branch 2 times, most recently from 517ede3 to 2b1b93b Compare August 5, 2020 13:17
@dominiklohmann dominiklohmann marked this pull request as ready for review August 5, 2020 14:27
@dominiklohmann
Copy link
Member

For the review: note that this is a breaking change for both MessagePack-encoded table slices in the archive and the persisted type-registry state.

@dominiklohmann dominiklohmann requested a review from a team August 5, 2020 14:28
@dominiklohmann dominiklohmann dismissed their stale review August 5, 2020 14:28

Dismissing my own review since I took the PR over.

@mavam
Copy link
Member Author

mavam commented Aug 5, 2020

Great. You can "self-approve" this and merge! 🎉

dominiklohmann
dominiklohmann approved these changes Aug 5, 2020
View reviewed changes
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

Approving after peer-review w/ @mavam.

@dominiklohmann dominiklohmann merged commit 087686d into master Aug 5, 2020
@dominiklohmann dominiklohmann deleted the story/ch18270 branch August 5, 2020 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dominiklohmann dominiklohmann dominiklohmann approved these changes

Assignees
No one assigned
Labels
bug Incorrect behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mavam @dominiklohmann