Skip to content

Conversation

practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Jul 26, 2018

Use explicit captures in lambda expressions.

Rationale:

  • Explicit is better than implicit.
  • Help avoid the lambda dangling reference problem (UB) by being explicit with what objects we use. Makes it easier to reason about their lifetimes.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 26, 2018

No more conflicts as of last run.

@practicalswift
Copy link
Contributor Author

Rebased!

@promag
Copy link
Contributor

promag commented Jul 27, 2018

I'm -0 on this. Even if this is merged, how could we prevent implicit captures in the future?

@practicalswift
Copy link
Contributor Author

@promag What are the best arguments in favour of implicit captures? :-)

@laanwj
Copy link
Member

laanwj commented Jul 30, 2018

Tend towards NACK. Sorry to rant, but I'm starting to feel quite strongly about this:

If you want to make changes like this, the way to do so is:

  • create a PR to update the coding guidelines with your motivation on how this avoids bugs (as you did in the OP), and that your point is strong enough for this to be merged
  • if it is merged, make sure it is applied to new code, in reviews
  • then maybe, in the longer term, if people agree that this is important, do a PR that updates old occurrences

Please don't create a PR out of the blue that changes some formerly unheard-of thing in the entire codebase. This creates review work, extra risk, and all massive idempotent code changes all over the code make it harder to keep track of what is actually changing.

@practicalswift practicalswift force-pushed the explicit-capturing-in-lambdas branch from 981b52d to a4def4d Compare July 30, 2018 12:32
@practicalswift
Copy link
Contributor Author

@promag Thanks for reviewing. I've now updated the PR. Implicit captures are now kept for trivial lambdas with only one capture. Explicit captures are used when the capture count is two or above. What do you think? Please re-review :-)

@practicalswift
Copy link
Contributor Author

@laanwj Makes sense! Closing this PR

@practicalswift practicalswift deleted the explicit-capturing-in-lambdas branch April 10, 2021 19:35
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants