Proposal: Allow Lambda capture expressions and the capture of raw pointers

193 views
Skip to first unread message

jdoe...@chromium.org

unread,
Jan 23, 2018, 9:52:23 AM1/23/18
to cxx
C++14 allows init-captures in lambdas [1] of the form 

auto widget = std::make_unique<Widget>();
auto lambda = [widget = std::move(widget)]() {
SetWidget(std::move(widget));
}

While it's usage should be low, it can be convenient for tests when combined with base::BindLambdaForTesting(). As I don't find a good reason to ban this feature I vote for allowing it.

At the same time I would like to allow the init-capture of raw pointers as well. Currently using an init-capture with a raw pointer triggers the "Do not use auto to deduce a raw pointer" check [2] and the following code does not compile:
auto widget = std::make_unique<Widget>();
auto lambda = [widget_raw = widget.get()]() {
UseWidget(*widget_raw);
}

A workaround exists in the following form, but seems less elegant to me:
auto widget = std::make_unique<Widget>();
auto lambda = [&widget_ref = *widget]() {
UseWidget(widget_ref);
}

As there is no explicit auto that is deduced to a raw pointer, I feel like this case qualifies as a false positive for the check and it should be allowed.

Please let me know what you think,
Jan

dan...@chromium.org

unread,
Jan 23, 2018, 10:55:26 AM1/23/18
to Jan Wilken, cxx
On Tue, Jan 23, 2018 at 9:52 AM, <jdoe...@chromium.org> wrote:
C++14 allows init-captures in lambdas [1] of the form 

auto widget = std::make_unique<Widget>();
auto lambda = [widget = std::move(widget)]() {
SetWidget(std::move(widget));
}

While it's usage should be low, it can be convenient for tests when combined with base::BindLambdaForTesting(). As I don't find a good reason to ban this feature I vote for allowing it.

I agree, allowing this seems reasonable.
 

At the same time I would like to allow the init-capture of raw pointers as well. Currently using an init-capture with a raw pointer triggers the "Do not use auto to deduce a raw pointer" check [2] and the following code does not compile:
auto widget = std::make_unique<Widget>();
auto lambda = [widget_raw = widget.get()]() {
UseWidget(*widget_raw);
}

A workaround exists in the following form, but seems less elegant to me:
auto widget = std::make_unique<Widget>();
auto lambda = [&widget_ref = *widget]() {
UseWidget(widget_ref);
}

As there is no explicit auto that is deduced to a raw pointer, I feel like this case qualifies as a false positive for the check and it should be allowed.

I think this is a bug in the implementation of the clang-plugin. Can you file a bug and maybe send a patch for it if you're so inclined?
 

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/c828815c-e8d2-468c-ae89-7627e8aab30f%40chromium.org.

Peter Kasting

unread,
Jan 23, 2018, 2:54:38 PM1/23/18
to jdoe...@chromium.org, cxx
On Tue, Jan 23, 2018 at 6:52 AM, <jdoe...@chromium.org> wrote:
C++14 allows init-captures in lambdas [1] of the form 

auto widget = std::make_unique<Widget>();
auto lambda = [widget = std::move(widget)]() {
SetWidget(std::move(widget));
}

While it's usage should be low, it can be convenient for tests when combined with base::BindLambdaForTesting(). As I don't find a good reason to ban this feature I vote for allowing it.

There has been controversy on the internal C-style threads about this.  No one really supports fully banning this feature, but Chandler Carruth in particular has deep reservations about it:

"...we should document that programmers should avoid init captures at all costs. If there is any other way to solve the problem at hand, that would be better. This features is powerful but deeply confusing in both syntax and resulting behavior. People will continually be surprised by the exact behavior and by the compiler error messages they get in the interim. I think we should minimize how much this is used in our codebase. ... I do see the utility of being able to move a unique_ptr into a lambda that becomes move-only itself. However, I find that the init-capture syntax is too high of a price. It is a completely novel syntax with an entirely new set of gotchas and rules that I don't ever want to use, teach, explain, or debug. Will there be places where it is the only remotely reasonable solution? Probably. I would like to wait until that very moment before using the feature."

AFAIK, the current stance is "allowed but very strongly discouraged".  If we want to allow these in Chromium, we should probably clarify if that's still the internal stance; understand why; and see if we can summarize the recommendations in our public documentation.  The public style guide is pretty much silent on this issue.

PK

dan...@chromium.org

unread,
Jan 23, 2018, 3:07:49 PM1/23/18
to Peter Kasting, Jan Wilken, cxx
Right, I looked there also and saw nothing. I don't understand what Chandler's concerns are here from the text you quoted. The syntax is assignment, it doesn't seem so out of place to me, anymore than the rest of the lambda syntax. Variable shadowing is "a bad thing" generally, so reusing the same variable name seems like a bad practice here just as it would in any other nested scope.

Chromium doesn't even use lambdas except for use in a local scope, or in tests. To me this seems harmless but I'd be curious to understand what his concerns are, maybe there are gotchas that were not in the documentation I read about the feature.
 

PK

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

Nico Weber

unread,
Jan 23, 2018, 3:11:08 PM1/23/18
to Dana Jansens, Peter Kasting, Jan Wilken, cxx
On Tue, Jan 23, 2018 at 3:07 PM, <dan...@chromium.org> wrote:
On Tue, Jan 23, 2018 at 2:54 PM, 'Peter Kasting' via cxx <c...@chromium.org> wrote:
On Tue, Jan 23, 2018 at 6:52 AM, <jdoe...@chromium.org> wrote:
C++14 allows init-captures in lambdas [1] of the form 

auto widget = std::make_unique<Widget>();
auto lambda = [widget = std::move(widget)]() {
SetWidget(std::move(widget));
}

While it's usage should be low, it can be convenient for tests when combined with base::BindLambdaForTesting(). As I don't find a good reason to ban this feature I vote for allowing it.

There has been controversy on the internal C-style threads about this.  No one really supports fully banning this feature, but Chandler Carruth in particular has deep reservations about it:

"...we should document that programmers should avoid init captures at all costs. If there is any other way to solve the problem at hand, that would be better. This features is powerful but deeply confusing in both syntax and resulting behavior. People will continually be surprised by the exact behavior and by the compiler error messages they get in the interim. I think we should minimize how much this is used in our codebase. ... I do see the utility of being able to move a unique_ptr into a lambda that becomes move-only itself. However, I find that the init-capture syntax is too high of a price. It is a completely novel syntax with an entirely new set of gotchas and rules that I don't ever want to use, teach, explain, or debug. Will there be places where it is the only remotely reasonable solution? Probably. I would like to wait until that very moment before using the feature."

AFAIK, the current stance is "allowed but very strongly discouraged".  If we want to allow these in Chromium, we should probably clarify if that's still the internal stance; understand why; and see if we can summarize the recommendations in our public documentation.  The public style guide is pretty much silent on this issue.

Right, I looked there also and saw nothing. I don't understand what Chandler's concerns are here from the text you quoted. The syntax is assignment, it doesn't seem so out of place to me, anymore than the rest of the lambda syntax.

My guess is that declaring a new variable without mentioning a type (or even `auto`) is the cause for concern. I find that pretty wild too.
 
Variable shadowing is "a bad thing" generally, so reusing the same variable name seems like a bad practice here just as it would in any other nested scope.

Chromium doesn't even use lambdas except for use in a local scope, or in tests. To me this seems harmless but I'd be curious to understand what his concerns are, maybe there are gotchas that were not in the documentation I read about the feature.
 

PK

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
 To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFA97mKWgUXRYHGtzAUqtY%2B8nsX_Mxnh2jR6Oa0JV4NMGg%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

dan...@chromium.org

unread,
Jan 23, 2018, 3:16:06 PM1/23/18
to Nico Weber, Peter Kasting, Jan Wilken, cxx
On Tue, Jan 23, 2018 at 3:11 PM, Nico Weber <tha...@chromium.org> wrote:
On Tue, Jan 23, 2018 at 3:07 PM, <dan...@chromium.org> wrote:
On Tue, Jan 23, 2018 at 2:54 PM, 'Peter Kasting' via cxx <c...@chromium.org> wrote:
On Tue, Jan 23, 2018 at 6:52 AM, <jdoe...@chromium.org> wrote:
C++14 allows init-captures in lambdas [1] of the form 

auto widget = std::make_unique<Widget>();
auto lambda = [widget = std::move(widget)]() {
SetWidget(std::move(widget));
}

While it's usage should be low, it can be convenient for tests when combined with base::BindLambdaForTesting(). As I don't find a good reason to ban this feature I vote for allowing it.

There has been controversy on the internal C-style threads about this.  No one really supports fully banning this feature, but Chandler Carruth in particular has deep reservations about it:

"...we should document that programmers should avoid init captures at all costs. If there is any other way to solve the problem at hand, that would be better. This features is powerful but deeply confusing in both syntax and resulting behavior. People will continually be surprised by the exact behavior and by the compiler error messages they get in the interim. I think we should minimize how much this is used in our codebase. ... I do see the utility of being able to move a unique_ptr into a lambda that becomes move-only itself. However, I find that the init-capture syntax is too high of a price. It is a completely novel syntax with an entirely new set of gotchas and rules that I don't ever want to use, teach, explain, or debug. Will there be places where it is the only remotely reasonable solution? Probably. I would like to wait until that very moment before using the feature."

AFAIK, the current stance is "allowed but very strongly discouraged".  If we want to allow these in Chromium, we should probably clarify if that's still the internal stance; understand why; and see if we can summarize the recommendations in our public documentation.  The public style guide is pretty much silent on this issue.

Right, I looked there also and saw nothing. I don't understand what Chandler's concerns are here from the text you quoted. The syntax is assignment, it doesn't seem so out of place to me, anymore than the rest of the lambda syntax.

My guess is that declaring a new variable without mentioning a type (or even `auto`) is the cause for concern. I find that pretty wild too.

I agree, but feel similarly about the rest of lambda syntax I guess. You have to read specifically about lambda syntax and understand it before you can tell what the code is doing, it doesn't read like any other C++.

Jeremy Roman

unread,
Jan 23, 2018, 4:14:38 PM1/23/18
to Dana Jansens, Nico Weber, Peter Kasting, Jan Wilken, cxx
The feature can be subtle and its type deduction rules are, from my reading of the internal thread, slightly different than you might naively expect (but this doesn't affect common cases).

Still, given that we discourage use of lambda capture in Chromium so much in general (more than Google does internally) in favour of base::Bind, are there many use cases that would be significantly improved by capture expressions? If the lambda isn't leaving the scope (this is the assumption behind all cases that we allow lambda capture at all, I believe), then in most cases you can fairly easily just capture the unique_ptr by reference with similar effect.

Given this one has proven controversial internally, I'm a little hesitant to allow it broadly if it's likely to cause us to diverge from upstream Google style going forward.

Peter Kasting

unread,
Jan 23, 2018, 4:23:12 PM1/23/18
to Dana Jansens, Jan Wilken, cxx
On Tue, Jan 23, 2018 at 12:07 PM, <dan...@chromium.org> wrote:
On Tue, Jan 23, 2018 at 2:54 PM, 'Peter Kasting' via cxx <c...@chromium.org> wrote:
On Tue, Jan 23, 2018 at 6:52 AM, <jdoe...@chromium.org> wrote:
C++14 allows init-captures in lambdas [1] of the form 

auto widget = std::make_unique<Widget>();
auto lambda = [widget = std::move(widget)]() {
SetWidget(std::move(widget));
}

While it's usage should be low, it can be convenient for tests when combined with base::BindLambdaForTesting(). As I don't find a good reason to ban this feature I vote for allowing it.

There has been controversy on the internal C-style threads about this.  No one really supports fully banning this feature, but Chandler Carruth in particular has deep reservations about it:

"...we should document that programmers should avoid init captures at all costs. If there is any other way to solve the problem at hand, that would be better. This features is powerful but deeply confusing in both syntax and resulting behavior. People will continually be surprised by the exact behavior and by the compiler error messages they get in the interim. I think we should minimize how much this is used in our codebase. ... I do see the utility of being able to move a unique_ptr into a lambda that becomes move-only itself. However, I find that the init-capture syntax is too high of a price. It is a completely novel syntax with an entirely new set of gotchas and rules that I don't ever want to use, teach, explain, or debug. Will there be places where it is the only remotely reasonable solution? Probably. I would like to wait until that very moment before using the feature."

AFAIK, the current stance is "allowed but very strongly discouraged".  If we want to allow these in Chromium, we should probably clarify if that's still the internal stance; understand why; and see if we can summarize the recommendations in our public documentation.  The public style guide is pretty much silent on this issue.

Right, I looked there also and saw nothing. I don't understand what Chandler's concerns are here from the text you quoted. The syntax is assignment, it doesn't seem so out of place to me, anymore than the rest of the lambda syntax. Variable shadowing is "a bad thing" generally, so reusing the same variable name seems like a bad practice here just as it would in any other nested scope.

Chromium doesn't even use lambdas except for use in a local scope, or in tests. To me this seems harmless but I'd be curious to understand what his concerns are, maybe there are gotchas that were not in the documentation I read about the feature.

I actually agree with you.  I think this feature is generally fine and would choose to allow it if it were up to me; and I don't really understand the objections.  That's why my hoped-for outcome was that someone figured out precisely what they were and how they fit into Chromium, or what the likely direction of the internal C arbiters would be in the future.  Maybe we could confidently say something like "because we only use lambdas in local scopes in Chromium, initializers in captures are fine".  Or maybe we couldn't.

PK

jdoe...@chromium.org

unread,
Jan 23, 2018, 5:41:41 PM1/23/18
to cxx, dan...@chromium.org, tha...@chromium.org, pkas...@google.com, jdoe...@chromium.org


On Tuesday, January 23, 2018 at 10:14:38 PM UTC+1, Jeremy Roman wrote:
The feature can be subtle and its type deduction rules are, from my reading of the internal thread, slightly different than you might naively expect (but this doesn't affect common cases).

Reading the internal thread it seems like Chandler wrongly assumed the type deduction rules are different between auto and init-captures with regard to braced init lists, but was later corrected by James Dennett. The rules are equivalent: http://eel.is/c++draft/expr.prim.lambda.capture#6 


Still, given that we discourage use of lambda capture in Chromium so much in general (more than Google does internally) in favour of base::Bind, are there many use cases that would be significantly improved by capture expressions? If the lambda isn't leaving the scope (this is the assumption behind all cases that we allow lambda capture at all, I believe), then in most cases you can fairly easily just capture the unique_ptr by reference with similar effect.

That is true, the usefulness of this feature is rather limited if lambdas don't leave the scope. Even the current notes on the Chromium C++11 / 14 page say so: "Particularly useful to capture move-only types in a lambda when a reference would go out of scope. Less useful without allowing lambdas to outlive the scope." 

However, I still think this feature can be useful for tests. In tests we already allow capturing lambdas to leave the scope via base::BindLambdaForTesting(). Maybe we can agree to allow init-captures only in tests? 

One use case where I would like to use this feature can be found in [1]. Here I am triggering a callback to run in the destructor of an object by resetting the only scoped_refptr pointing to it. In the callback I am referring to the not yet destructed object and using methods of the test fixture, thus I also need to capture `this`. A lambda simply capturing the refptr by reference wouldn't work, since by the time the callback runs the refptr is already set to null. Hence I need to capture a different pointer / reference to the actual object. This example might be a bit contrived, and capturing unique_ptrs by move is likely a more common use case.

Jeremy Roman

unread,
Jan 23, 2018, 6:10:25 PM1/23/18
to Jan Wilken, cxx, Dana Jansens, Nico Weber, Peter Kasting
On Tue, Jan 23, 2018 at 5:41 PM, <jdoe...@chromium.org> wrote:


On Tuesday, January 23, 2018 at 10:14:38 PM UTC+1, Jeremy Roman wrote:
The feature can be subtle and its type deduction rules are, from my reading of the internal thread, slightly different than you might naively expect (but this doesn't affect common cases).

Reading the internal thread it seems like Chandler wrongly assumed the type deduction rules are different between auto and init-captures with regard to braced init lists, but was later corrected by James Dennett. The rules are equivalent: http://eel.is/c++draft/expr.prim.lambda.capture#6 

Ah, I believe you are correct.
 

Still, given that we discourage use of lambda capture in Chromium so much in general (more than Google does internally) in favour of base::Bind, are there many use cases that would be significantly improved by capture expressions? If the lambda isn't leaving the scope (this is the assumption behind all cases that we allow lambda capture at all, I believe), then in most cases you can fairly easily just capture the unique_ptr by reference with similar effect.

That is true, the usefulness of this feature is rather limited if lambdas don't leave the scope. Even the current notes on the Chromium C++11 / 14 page say so: "Particularly useful to capture move-only types in a lambda when a reference would go out of scope. Less useful without allowing lambdas to outlive the scope." 

However, I still think this feature can be useful for tests. In tests we already allow capturing lambdas to leave the scope via base::BindLambdaForTesting(). Maybe we can agree to allow init-captures only in tests? 

One use case where I would like to use this feature can be found in [1]. Here I am triggering a callback to run in the destructor of an object by resetting the only scoped_refptr pointing to it. In the callback I am referring to the not yet destructed object and using methods of the test fixture, thus I also need to capture `this`. A lambda simply capturing the refptr by reference wouldn't work, since by the time the callback runs the refptr is already set to null. Hence I need to capture a different pointer / reference to the actual object. This example might be a bit contrived, and capturing unique_ptrs by move is likely a more common use case.


ISTM that this is equivalent (capturing a raw pointer or reference, by reference):

  bool error_callback_called = false;
  BluetoothAdapter& adapter = *adapter_;
  adapter.SetPowered(
      false, GetCallback(Call::NOT_EXPECTED),
      base::BindLambdaForTesting(
          // Note that we explicitly need to capture a reference to the
          // underlying adapter, even though we pass |this| implicitly. This is
          // because by the time this callback is invoked, |adapter_| is already
          // set to null, but the pointed to adapter instance is still alive. So
          // using the reference is safe, but dereferencing |adapter_| crashes.
          [&] {
            error_callback_called = true;
            adapter.SetPowered(false, GetCallback(Call::NOT_EXPECTED),
                               GetErrorCallback(Call::EXPECTED));
            adapter.SetPowered(true, GetCallback(Call::NOT_EXPECTED),
                               GetErrorCallback(Call::EXPECTED));
            
          }));
 
I'm not sure I see a compelling readability difference between the two.

jdoe...@chromium.org

unread,
Jan 23, 2018, 8:59:34 PM1/23/18
to cxx, jdoe...@chromium.org, dan...@chromium.org, tha...@chromium.org, pkas...@google.com
True, fair point. Arguably your version is even more readable, I should have thought of that myself. 

Jan Wilken Dörrie

unread,
May 14, 2018, 5:31:51 AM5/14/18
to cxx, dan...@chromium.org, tha...@chromium.org, pkas...@google.com
I would like to revive this thread, as I found another usecase of this feature. I am currently working with Microsoft's UWP [1] APIs, which make use of async operations via the IAsyncOperation interface [2]. This interface has the concept of completion handlers, which will be invoked with the result once the operation is done. Using WRL's Callback [3] class, these handlers can be constructed from any callable, i.e. Lambas, Functors, function pointers and std::function objects [4]. In order to simplify using this API with existing code, I created a wrapper function that takes a IAsyncOperation and a base::OnceCallback. In the function body I then create a WRL::Callback instance, constructed from a move-capturing lambda, that runs the captured OnceCallback once the lambda is invoked. Admittedly, here the lambda does leave the local scope, but it is the most elegant solution. Since it is possible to work around a ban of move captures by either creating an appropriate functor class or using a lambda with base::AdaptCallbackForRepeating, banning this feature does not result in reduced functionality. Move-capturing lambdas are simply the most concise (and arguably the most elegant) way to achieve the desired functionality. Because of this I would like to formally allow lambda capture expressions. However, I am perfectly fine with adding disclaimers that usage should be (very) rare, and that it should be restricted to move-captures, as value- and reference-captures can be achieved in other ways.

Best,

Jeremy Roman

unread,
May 14, 2018, 3:27:01 PM5/14/18
to Jan Wilken Dörrie, cxx, Dana Jansens, Nico Weber, Peter Kasting
On Mon, May 14, 2018 at 5:31 AM, Jan Wilken Dörrie <jdoe...@chromium.org> wrote:
I would like to revive this thread, as I found another usecase of this feature. I am currently working with Microsoft's UWP [1] APIs, which make use of async operations via the IAsyncOperation interface [2]. This interface has the concept of completion handlers, which will be invoked with the result once the operation is done. Using WRL's Callback [3] class, these handlers can be constructed from any callable, i.e. Lambas, Functors, function pointers and std::function objects [4]. In order to simplify using this API with existing code, I created a wrapper function that takes a IAsyncOperation and a base::OnceCallback. In the function body I then create a WRL::Callback instance, constructed from a move-capturing lambda, that runs the captured OnceCallback once the lambda is invoked. Admittedly, here the lambda does leave the local scope, but it is the most elegant solution. Since it is possible to work around a ban of move captures by either creating an appropriate functor class or using a lambda with base::AdaptCallbackForRepeating, banning this feature does not result in reduced functionality.

It's possible to work around basically all of our style rules.
 
Move-capturing lambdas are simply the most concise (and arguably the most elegant) way to achieve the desired functionality. Because of this I would like to formally allow lambda capture expressions. However, I am perfectly fine with adding disclaimers that usage should be (very) rare, and that it should be restricted to move-captures, as value- and reference-captures can be achieved in other ways.

I wouldn't feel that bad about allowing this, though on the other hand it doesn't seem like the workaround is that bad either, especially since it sounds like you intend to isolate it to one adapter function. e.g. this seems like it suffices:

template <typename C>
struct WRLCallbackAdapter {
  template <typename... Args>
  decltype(auto) operator()(Args&&... args) {
    return std::move(callback).Run(std::forward<Args>(args)...);
  }

  C callback;
};

template <typename TDelegateInterface, typename C>
Microsoft::WRL::ComPtr<TDelegateInterface> AsWRLCallback(C cb) {
  return Microsoft::WRL::Callback<TDelegateInterface>(
      WRLCallbackAdapter<C>{std::move(cb)});
}

Right? (Aside: root of the problem seems to be that we call our method Run instead of operator(), like most C++ code does. But for that, it looks to me like at least base::RepeatingCallback would Just Work™.)

I assume the code you intend to write is roughly:

template <typename TDelegateInterface, typename C>
Microsoft::WRL::ComPtr<TDelegateInterface> AsWRLCallback(C cb) {
  return Microsoft::WRL::Callback<TDelegateInterface>([cb = std::move(cb)](
      auto&&... args) mutable {
    return std::move(cb).Run(std::forward<decltype(args)>(args)...);
  });
}

I agree that it's slightly nicer, but unless more such cases come up, the way things are doesn't seem awful.

Jan Wilken Dörrie

unread,
May 14, 2018, 3:42:10 PM5/14/18
to Jeremy Roman, cxx, Dana Jansens, Nico Weber, Peter Kasting
The code using it landed today: https://codesearch.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter_winrt.cc?l=112-123&rcl=16a2164f7370d08a07206bcb2001668e56269af5
I do a few other move captures, but these are mostly to not necessarily add and remove references to ref-counted objects. Of course, all of this could be done with a corresponding functor as well.

dan...@chromium.org

unread,
May 10, 2019, 2:03:34 PM5/10/19
to Peter Kasting, Jan Wilken, cxx
I think we should. 

I don't think we need to ban C++ things based on syntax being weird, or we'd have no C++ left. Capturing lambdas are indeed very limited in our code-base, and have very controlled scope outside of tests. So I think these should be allowed.

- Dana
 

PK

Jeremy Roman

unread,
May 10, 2019, 3:53:37 PM5/10/19
to Dana Jansens, Peter Kasting, Jan Wilken, cxx
I generally like this feature. My only reservation is if it looks like Google C++ is likely to ban it. Assuming not, I'm on board with allowing with the same caveats as other capturing lambdas.
 
- Dana
 

PK

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

Giovanni Ortuño

unread,
May 13, 2019, 9:48:46 PM5/13/19
to Jeremy Roman, Dana Jansens, Peter Kasting, Jan Wilken, cxx
From: Jeremy Roman <jbr...@chromium.org>
Date: Sat, May 11, 2019 at 5:53 AM
To: Dana Jansens
Cc: Peter Kasting, Jan Wilken, cxx



On Fri, May 10, 2019 at 2:03 PM <dan...@chromium.org> wrote:
On Tue, Jan 23, 2018 at 4:23 PM Peter Kasting <pkas...@google.com> wrote:
On Tue, Jan 23, 2018 at 12:07 PM, <dan...@chromium.org> wrote:
On Tue, Jan 23, 2018 at 2:54 PM, 'Peter Kasting' via cxx <c...@chromium.org> wrote:
On Tue, Jan 23, 2018 at 6:52 AM, <jdoe...@chromium.org> wrote:
C++14 allows init-captures in lambdas [1] of the form 

auto widget = std::make_unique<Widget>();
auto lambda = [widget = std::move(widget)]() {
SetWidget(std::move(widget));
}

While it's usage should be low, it can be convenient for tests when combined with base::BindLambdaForTesting(). As I don't find a good reason to ban this feature I vote for allowing it.

There has been controversy on the internal C-style threads about this.  No one really supports fully banning this feature, but Chandler Carruth in particular has deep reservations about it:

"...we should document that programmers should avoid init captures at all costs. If there is any other way to solve the problem at hand, that would be better. This features is powerful but deeply confusing in both syntax and resulting behavior. People will continually be surprised by the exact behavior and by the compiler error messages they get in the interim. I think we should minimize how much this is used in our codebase. ... I do see the utility of being able to move a unique_ptr into a lambda that becomes move-only itself. However, I find that the init-capture syntax is too high of a price. It is a completely novel syntax with an entirely new set of gotchas and rules that I don't ever want to use, teach, explain, or debug. Will there be places where it is the only remotely reasonable solution? Probably. I would like to wait until that very moment before using the feature."

AFAIK, the current stance is "allowed but very strongly discouraged".  If we want to allow these in Chromium, we should probably clarify if that's still the internal stance; understand why; and see if we can summarize the recommendations in our public documentation.  The public style guide is pretty much silent on this issue.

Right, I looked there also and saw nothing. I don't understand what Chandler's concerns are here from the text you quoted. The syntax is assignment, it doesn't seem so out of place to me, anymore than the rest of the lambda syntax. Variable shadowing is "a bad thing" generally, so reusing the same variable name seems like a bad practice here just as it would in any other nested scope.

Chromium doesn't even use lambdas except for use in a local scope, or in tests. To me this seems harmless but I'd be curious to understand what his concerns are, maybe there are gotchas that were not in the documentation I read about the feature.

I actually agree with you.  I think this feature is generally fine and would choose to allow it if it were up to me; and I don't really understand the objections.  That's why my hoped-for outcome was that someone figured out precisely what they were and how they fit into Chromium, or what the likely direction of the internal C arbiters would be in the future.  Maybe we could confidently say something like "because we only use lambdas in local scopes in Chromium, initializers in captures are fine".  Or maybe we couldn't.

I think we should. 

I don't think we need to ban C++ things based on syntax being weird, or we'd have no C++ left. Capturing lambdas are indeed very limited in our code-base, and have very controlled scope outside of tests. So I think these should be allowed.

I generally like this feature. My only reservation is if it looks like Google C++ is likely to ban it. Assuming not, I'm on board with allowing with the same caveats as other capturing lambdas.

I'm not sure how we can get an answer from the internal arbiters either way until they start using C++14 internally. In the last thread the feature was discussed they seem to imply that the feature is up for discussion until then.
 
 
- Dana
 

PK

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaR722G3-FfDsGocqHziL4VC%3DEnWENHFJ7cJut1UnuuvRQ%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

Jan Wilken Dörrie

unread,
May 16, 2019, 5:40:45 AM5/16/19
to Giovanni Ortuño, Jeremy Roman, Dana Jansens, Peter Kasting, cxx
I reached out to the internal style arbiters and it looks like they will allow capture expressions with the following restrictions:
  • Only use capture expressions if direct capture is not possible. Example:
    int foo = 42;
    auto lambda = [foo=foo] {...};
    // Bad: Just use [foo] {...} instead.

    auto bar = std::make_unique<int>(42);
    auto lambda = [bar=std::move(bar)] {...};
    // Good: Just using [bar] {...} is not possible.

    Note that this allows using capture expressions to capture data members by value, which wasn't possible before.

  • Don't use init captures to introduce new names, i.e. always shadow existing variables (or data members). Example:
    class     C {
      int member_ = 42;
      void foo() {
        auto lambda = [member=member_] {...};
        // Bad: member is a new name. Use [member_=member_] {...}; instead.
      }
    };
Both of these rules seem very reasonable to me and match how I would have used capture expressions intuitively. Furthermore, rule two seems to address Nico's concern that init-captures allow you to introduce new variables without specifying any type declarations. Technically we are still introducing new variables, but they will have the same name and a similar type (same % cv-ref qualifiers) as the old variable.

I thus propose allowing init-captures with the caveat that their usage should be rare, and that the above rules should be followed. If no-one has strong objections against this, I will send out a CL to this effect shortly.

Cheers,
Jan

Jeremy Roman

unread,
May 16, 2019, 10:22:41 AM5/16/19
to Jan Wilken Dörrie, Giovanni Ortuño, Dana Jansens, Peter Kasting, cxx
On Thu, May 16, 2019 at 5:40 AM Jan Wilken Dörrie <jdoe...@chromium.org> wrote:
I reached out to the internal style arbiters and it looks like they will allow capture expressions with the following restrictions:
  • Only use capture expressions if direct capture is not possible. Example:
    int foo = 42;
    auto lambda = [foo=foo] {...};
    // Bad: Just use [foo] {...} instead.

    auto bar = std::make_unique<int>(42);
    auto lambda = [bar=std::move(bar)] {...};
    // Good: Just using [bar] {...} is not possible.

    Note that this allows using capture expressions to capture data members by value, which wasn't possible before.

  • Don't use init captures to introduce new names, i.e. always shadow existing variables (or data members). Example:
    class     C {
      int member_ = 42;
      void foo() {
        auto lambda = [member=member_] {...};
        // Bad: member is a new name. Use [member_=member_] {...}; instead.
      }
    };
Both of these rules seem very reasonable to me and match how I would have used capture expressions intuitively. Furthermore, rule two seems to address Nico's concern that init-captures allow you to introduce new variables without specifying any type declarations. Technically we are still introducing new variables, but they will have the same name and a similar type (same % cv-ref qualifiers) as the old variable.

I thus propose allowing init-captures with the caveat that their usage should be rare, and that the above rules should be followed. If no-one has strong objections against this, I will send out a CL to this effect shortly.

sgtm (though I do personally find the second rule a little odd)

dan...@chromium.org

unread,
May 16, 2019, 11:33:08 AM5/16/19
to Jeremy Roman, Jan Wilken Dörrie, Giovanni Ortuño, Peter Kasting, cxx
On Thu, May 16, 2019 at 10:22 AM Jeremy Roman <jbr...@chromium.org> wrote:


On Thu, May 16, 2019 at 5:40 AM Jan Wilken Dörrie <jdoe...@chromium.org> wrote:
I reached out to the internal style arbiters and it looks like they will allow capture expressions with the following restrictions:
  • Only use capture expressions if direct capture is not possible. Example:
    int foo = 42;
    auto lambda = [foo=foo] {...};
    // Bad: Just use [foo] {...} instead.

    auto bar = std::make_unique<int>(42);
    auto lambda = [bar=std::move(bar)] {...};
    // Good: Just using [bar] {...} is not possible.

    Note that this allows using capture expressions to capture data members by value, which wasn't possible before.

  • Don't use init captures to introduce new names, i.e. always shadow existing variables (or data members). Example:
    class     C {
      int member_ = 42;
      void foo() {
        auto lambda = [member=member_] {...};
        // Bad: member is a new name. Use [member_=member_] {...}; instead.
      }
    };
Both of these rules seem very reasonable to me and match how I would have used capture expressions intuitively. Furthermore, rule two seems to address Nico's concern that init-captures allow you to introduce new variables without specifying any type declarations. Technically we are still introducing new variables, but they will have the same name and a similar type (same % cv-ref qualifiers) as the old variable.

I thus propose allowing init-captures with the caveat that their usage should be rare, and that the above rules should be followed. If no-one has strong objections against this, I will send out a CL to this effect shortly.

sgtm (though I do personally find the second rule a little odd)

It is, but it SGTM too. I'd change the "good" second example to be something we'd actually allow instead of being a "bad" int case though :)

lo...@chromium.org

unread,
May 16, 2019, 8:31:41 PM5/16/19
to cxx, jbr...@chromium.org, jdoe...@chromium.org, ort...@chromium.org, pkas...@google.com
Hi, all!

SGTM.
And [member=member_] looks reasonable: member is a deep copy of member_ (i.e. member is a detached snapshot of member_).

Alexey.

Jan Wilken Dörrie

unread,
May 17, 2019, 5:38:46 AM5/17/19
to lo...@chromium.org, cxx, Jeremy Roman, Giovanni Ortuño, Peter Kasting
Sent out Style Guide CL: https://crrev.com/c/1617506
Reply all
Reply to author
Forward
0 new messages