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

try to resize when serializing #7

Closed
wants to merge 1 commit into from

Conversation

wjwwood
Copy link
Contributor

@wjwwood wjwwood commented Jul 5, 2016

Like the deserialize functions, try to resize the buffer before giving up on serialization.

Connects to ros2/rmw_fastrtps#36.

@richiprosima
Copy link
Contributor

@wjwwood I was checking the PR. You are trying to resize the buffer in deserialize functions. Why do you need that? Your commit message says you are adding it in serialize functions, but I don't see this in the diff.

@wjwwood
Copy link
Contributor Author

wjwwood commented Jul 8, 2016

Sorry the message should have been deserializing I guess. I was probably meaning that it is needed during "serializing activities", which to me includes both serialization and deserialization. Sorry for the confusion on that part.

I can see how this doesn't make sense... In fact, it might have only been necessary to cover up a bug. You'd only want to resize during deserialize if you were asking to extract more data than was in the original octet stream, which shouldn't happen...

I think I'll close this and redo my testing without it. However, perhaps these error messages should be something different:

https://github.com/eProsima/Fast-CDR/pull/7/files#diff-7bab8e060e5362ab24fd4b3fa4e57525R1054

In this case, it's not a problem of not enough memory, but more like an out of bounds error, where the deserialization would require more data out of the array than is left.

@wjwwood wjwwood closed this Jul 8, 2016
@wjwwood wjwwood deleted the large_message_pub_sub branch July 8, 2016 21:13
@wjwwood
Copy link
Contributor Author

wjwwood commented Jul 8, 2016

It seems to work without this pr. Thanks for catching that.

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.

2 participants