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

Ensure the async_write lambda is a closure around the buffer #1231

Merged
merged 2 commits into from
Sep 25, 2018

Conversation

rkeene
Copy link
Contributor

@rkeene rkeene commented Sep 25, 2018

No description provided.

@rkeene rkeene added the bug label Sep 25, 2018
@rkeene rkeene added this to the V16.1 milestone Sep 25, 2018
@rkeene rkeene self-assigned this Sep 25, 2018
@rkeene
Copy link
Contributor Author

rkeene commented Sep 25, 2018

I can't approve my own pull request !

Copy link
Contributor

@cryptocode cryptocode left a comment

Choose a reason for hiding this comment

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

LGTM, with this change, some explicit captures in call-sites can probably be removed, like
https://github.com/nanocurrency/raiblocks/blob/5816c25422d066f9877d5cf3271489bd18853891/rai/node/bootstrap.cpp#L430

Since the capture isn't actually used, it's only there to extend the lifetime of the buffer, so the issue was probably known and just missed elsewhere (which this PR fixes for all cases)

Copy link
Contributor

@clemahieu clemahieu left a comment

Choose a reason for hiding this comment

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

This could be done inside async_write so it can’t be missed.

@rkeene
Copy link
Contributor Author

rkeene commented Sep 25, 2018

@clemahieu 👍 That's the first part of the changeset, it just looks like the others so it's easy to miss

Copy link
Contributor

@clemahieu clemahieu left a comment

Choose a reason for hiding this comment

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

LGTM

@rkeene rkeene merged commit 2b823e6 into master Sep 25, 2018
@rkeene rkeene deleted the lambda-closure-buffer branch September 26, 2018 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants