c++11: Allow lambdas, with some restrictions.

c++11: Allow lambdas, with some restrictions.

Thanks to David Michael <dmichael@chromium.org>; for condensing the discussion
thread to a recommendation!

BUG=none

Committed: https://crrev.com/a907c318b52d7b66d2a66837bdfa6faf9e1058a3
Cr-Commit-Position: refs/heads/master@{#302708}

[Nico, 2014-11-04 20:56:53]

thakis@chromium.org changed reviewers:
+ jamesr@chromium.org

[jamesr, 2014-11-04 21:06:29]

Can we get an example use in the patch as well?

[jamesr, 2014-11-04 21:08:49]

https://codereview.chromium.org/699283002/diff/20001/styleguide/c++/c++11.html
File styleguide/c++/c++11.html (right):

https://codereview.chromium.org/699283002/diff/20001/styleguide/c++/c++11.htm...
styleguide/c++/c++11.html:123: <td>Only use lambdas in cases where they will run
in the current thread of
language feels a little weaker than we intend - a lot of Bind() uses are to
create a callback that will run in the same thread, but asynchronously, which is
what we wanna ban here right?

[Nico, 2014-11-04 21:10:13]

On 2014/11/04 21:06:29, jamesr wrote:
> Can we get an example use in the patch as well?

That's a great suggestion. I did try that this works in at least some cases here
https://codereview.chromium.org/705473002/ but I'll try to find a place where
this makes sense and will add that to this patch.

[Peter Kasting, 2014-11-04 21:10:35]

pkasting@chromium.org changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2014-11-04 21:10:36]

https://codereview.chromium.org/699283002/diff/20001/styleguide/c++/c++11.html
File styleguide/c++/c++11.html (right):

https://codereview.chromium.org/699283002/diff/20001/styleguide/c++/c++11.htm...
styleguide/c++/c++11.html:123: <td>Only use lambdas in cases where they will run
in the current thread of
On 2014/11/04 21:08:49, jamesr wrote:
> language feels a little weaker than we intend - a lot of Bind() uses are to
> create a callback that will run in the same thread, but asynchronously, which
is
> what we wanna ban here right?

We explicitly say "Do not bind or store lambdas" right after this.

[Nico, 2014-11-04 21:12:49]

https://codereview.chromium.org/699283002/diff/20001/styleguide/c++/c++11.html
File styleguide/c++/c++11.html (right):

https://codereview.chromium.org/699283002/diff/20001/styleguide/c++/c++11.htm...
styleguide/c++/c++11.html:123: <td>Only use lambdas in cases where they will run
in the current thread of
On 2014/11/04 21:08:49, jamesr wrote:
> language feels a little weaker than we intend - a lot of Bind() uses are to
> create a callback that will run in the same thread, but asynchronously, which
is
> what we wanna ban here right?

It says "Do not bind or store lambdas" a few lines below. Should that be in
front? And without "store", running on different threads isn't possible anyways
I suppose. What about:

"Do not bind or store lambdas; use <code>base::Bind</code> and
<code>base::Callback</code>
instead, because they offer protection against a large class of object lifetime
mistakes. Don't use
default captures (<code>[=]</code>, <code>[&amp;]</code> &ndash; <a
href="https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Lambda_expressions">Google
Style Guide</a>). For example, a lambda may be used as a functor parameter to
methods or functions that will use it immediately, such as those in
<code>&lt;algorithm&gt;</code>."

[jamesr, 2014-11-04 21:14:18]

That's better, although storing on the stack should be fine e.g.

auto my_lambda = ....;
std::for_each(...., my_lambda);

right?

[Peter Kasting, 2014-11-04 21:15:37]

On 2014/11/04 21:12:49, Nico wrote:
> https://codereview.chromium.org/699283002/diff/20001/styleguide/c++/c++11.html
> File styleguide/c++/c++11.html (right):
> 
>
https://codereview.chromium.org/699283002/diff/20001/styleguide/c++/c++11.htm...
> styleguide/c++/c++11.html:123: <td>Only use lambdas in cases where they will
run
> in the current thread of
> On 2014/11/04 21:08:49, jamesr wrote:
> > language feels a little weaker than we intend - a lot of Bind() uses are to
> > create a callback that will run in the same thread, but asynchronously,
which
> is
> > what we wanna ban here right?
> 
> It says "Do not bind or store lambdas" a few lines below. Should that be in
> front? And without "store", running on different threads isn't possible
anyways
> I suppose. What about:
> 
> "Do not bind or store lambdas; use <code>base::Bind</code> and
> <code>base::Callback</code>
> instead, because they offer protection against a large class of object
lifetime
> mistakes. Don't use
> default captures (<code>[=]</code>, <code>[&amp;]</code> &ndash; <a
>
href="https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Lambda_expressions">Google
> Style Guide</a>). For example, a lambda may be used as a functor parameter to
> methods or functions that will use it immediately, such as those in
> <code>&lt;algorithm&gt;</code>."

Putting "For example" right after the restrictions seems confusing.  How about
this last sentence: "Lambdas are typically useful as a parameter to methods or
functions that will use them immediately, such as those in <algorithm>."

[Nico, 2014-11-04 21:31:08]

Added a few uses (clang-format understands lambdas, but in 2 of the 3 uses it
formatted them other than I would have). Applied pkasting's suggestion for the
last sentence.

On 2014/11/04 21:14:18, jamesr wrote:
> That's better, although storing on the stack should be fine e.g.
> 
> auto my_lambda = ....;
> std::for_each(...., my_lambda);
> 
> right?

I think this should be allowed; in practice I expect that having a real function
if the lambda's too long to be mentioned inline will often be what you want (for
testability etc). Do you think people will read "do not store" as "do not store
on stack"? Suggestions for rewording this? (I think as-is might be ok for
starters?)

[jamesr, 2014-11-04 22:42:36]

lgtm

[Nico, 2014-11-04 22:51:20]

The CQ bit was checked by thakis@chromium.org

[Nico, 2014-11-04 22:51:24]

The CQ bit was unchecked by thakis@chromium.org

[Nico, 2014-11-04 22:51:29]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2014-11-04 22:52:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/699283002/40001

[commit-bot: I haz the power, 2014-11-04 23:38:17]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-11-04 23:39:16]

Patchset 3 (id:??) landed as
https://crrev.com/a907c318b52d7b66d2a66837bdfa6faf9e1058a3
Cr-Commit-Position: refs/heads/master@{#302708}