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

Magic_numbers in calculateMaxSerializedSize #72

Closed
mikaelarguedas opened this issue Dec 7, 2016 · 9 comments
Closed

Magic_numbers in calculateMaxSerializedSize #72

mikaelarguedas opened this issue Dec 7, 2016 · 9 comments
Labels
enhancement New feature or request

Comments

@mikaelarguedas
Copy link
Member

mikaelarguedas commented Dec 7, 2016

This is a follow-up of #36
There are still magic numbers (101, 257 and 100) use to determine the maximum size of a sample.
It would be good to investigate if we can get rid of them and improve the logic of the MaxSize calculation

@mikaelarguedas mikaelarguedas added the enhancement New feature or request label Dec 7, 2016
@mikaelarguedas mikaelarguedas self-assigned this Dec 7, 2016
@mikaelarguedas mikaelarguedas added in progress Actively being worked on (Kanban column) ready Work is about to start (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Dec 7, 2016
@mikaelarguedas mikaelarguedas removed the ready Work is about to start (Kanban column) label Jan 4, 2017
@mikaelarguedas mikaelarguedas removed their assignment Jan 4, 2017
@allenh1
Copy link
Contributor

allenh1 commented Jun 1, 2017

@mikaelarguedas This might also be something I could work on

@mikaelarguedas
Copy link
Member Author

@allenh1 Is this one still on your radar ?

@allenh1
Copy link
Contributor

allenh1 commented Aug 4, 2017

Yep -- I was waiting on the refactor to finish, but looks like I can go after it now.

@allenh1 allenh1 added the ready Work is about to start (Kanban column) label Aug 9, 2017
@allenh1 allenh1 added in progress Actively being worked on (Kanban column) and removed ready Work is about to start (Kanban column) labels Aug 10, 2017
@mikaelarguedas
Copy link
Member Author

There is now only one magic number left.

@richiprosima I see there is a todo in your name to modify it, do you have any insight as of if this variable should be kept ?

@richiprosima
Copy link
Contributor

That value is inherited from when there was only preallocated memory policy in FastRTPS, and dynamic types had to have a maximum length. For sequences/arrays the maximum length was 100. Now ROS2 uses preallocated_with_realloc memory policy and maximum lengths are not needed.

@dirk-thomas
Copy link
Member

@richiprosima can you please propose a patch how to get rid of this variable. It isn't clear to us how to remove it cleanly.

@richiware
Copy link
Contributor

I'm working on it

@richiware
Copy link
Contributor

PR #152 removes those magic numbers.

@mikaelarguedas mikaelarguedas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Sep 12, 2017
@mikaelarguedas
Copy link
Member Author

All magic numbers have now been removed 🎉 closing

@mikaelarguedas mikaelarguedas removed the in review Waiting for review (Kanban column) label Sep 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants