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

Improve net/buffer #155

Merged
merged 13 commits into from Sep 4, 2015

Conversation

Projects
None yet
2 participants
@bassosimone
Member

bassosimone commented Aug 31, 2015

This pull request aims to simplify and to make more robust net/buffer code. More details in the individual commit messages. This is what was needed to prepare this part for version 0.1.

bassosimone added some commits Aug 29, 2015

Remove over engineering
1) no need to implement readpeek() as a template

2) no need to write std::vector<char>
Fix three memory errors
1) Return to an API using (const void *, size_t) rather than
   using (const char *, size_t) for a rather stupid reason: it
   seems that Catch is looking inside C strings assuming they
   are zero terminated, which is not the case for

       buffer.write(count, [](const char *, size_t) {})

   because of course in that case we pass around just allocated
   data but not initialized. Hence the memory errors.

   Passing `void *` fixes the issue because Catch does not
   of course look inside a `void *`.

2) Make sure we create `std::unique_ptr<evbuffer_iovec[]>`
   otherwise `delete` (rather than `delete[]`) is called

3) Make sure we delete what was allocated before raising an
   exception in `buffer.write(n, [](const char *, size_t) {})`

@bassosimone bassosimone added this to the measurement-kit 0.1.0 milestone Aug 31, 2015

if (evbuffer_add_buffer(evbuf, source) != 0)
throw std::runtime_error("evbuffer_add_buffer failed");
return (*this);
return *this;

This comment has been minimized.

@hellais

hellais Sep 2, 2015

Contributor

Just to understand, is this just a stylistic change or does it change the meaning?

@hellais

hellais Sep 2, 2015

Contributor

Just to understand, is this just a stylistic change or does it change the meaning?

This comment has been minimized.

@hellais

hellais Sep 2, 2015

Contributor

nevermind I didn't read the commit log (cosmetic changes)...

@hellais

hellais Sep 2, 2015

Contributor

nevermind I didn't read the commit log (cosmetic changes)...

@hellais

This comment has been minimized.

Show comment
Hide comment
@hellais

hellais Sep 2, 2015

Contributor

Skimming over the code I don't see any reason why not to merge. In the next days I will test it also by running it, but if I fail to do so soon, you should feel free to merge it.

Contributor

hellais commented Sep 2, 2015

Skimming over the code I don't see any reason why not to merge. In the next days I will test it also by running it, but if I fail to do so soon, you should feel free to merge it.

@bassosimone

This comment has been minimized.

Show comment
Hide comment
@bassosimone

bassosimone Sep 2, 2015

Member

Thank you!

Member

bassosimone commented Sep 2, 2015

Thank you!

@hellais hellais referenced this pull request Sep 2, 2015

Merged

Improve net/transport code #156

@bassosimone

This comment has been minimized.

Show comment
Hide comment
@bassosimone

bassosimone Sep 4, 2015

Member

Merging!

Member

bassosimone commented Sep 4, 2015

Merging!

bassosimone added a commit that referenced this pull request Sep 4, 2015

@bassosimone bassosimone merged commit fbb7f66 into measurement-kit:master Sep 4, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bassosimone bassosimone deleted the bassosimone:feature/net-buffer branch Sep 4, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment