Can we allow std::function in Chrome?

[W. James MacLean]

This would allow us to simplify use of closures for late-binding of IDs to IPCs in WebView-tag, as is described in https://bugs.chromium.org/p/chromium/issues/detail?id=544212

Cheers,

James

⭘ W. James MacLean ⭘ Software Engineer ⭘ Google Waterloo, Canada

[Jeremy Roman]

Does base::Callback (Chromium's existing counterpart to std::function) work for your use case?

--
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/CADAYvoc172Qpx_XygMzS3A9i7qUFWvrpbBHCm3L_NX7gP2V8MQ%40mail.gmail.com.

[Dana Jansens]

Can you explain why base::Bind is insufficient? We already have a way to make callable objects (Bind) and would prefer to not introduce a second.

On Wed, May 11, 2016 at 12:37 PM, 'W. James MacLean' via cxx <c...@chromium.org> wrote:

[Peter Kasting]

On Wed, May 11, 2016 at 12:40 PM, Dana Jansens <dan...@chromium.org> wrote:

Can you explain why base::Bind is insufficient? We already have a way to make callable objects (Bind) and would prefer to not introduce a second.

Speaking of which, is there a doc somewhere that summarizes the differences between the Chrome and STL mechanisms, especially with an eye towards why we would or would not ever move to the STL versions some day?

PK 

[W. James MacLean]

It's not that base::Bind is insufficient, but the code ends up looking simpler with std::function. I thought since we now allow closures, this would complement them nicely.

⭘ W. James MacLean ⭘ Software Engineer ⭘ Google Waterloo, Canada

[Dana Jansens]

On Wed, May 11, 2016 at 1:02 PM, W. James MacLean <wjma...@google.com> wrote:

It's not that base::Bind is insufficient, but the code ends up looking simpler with std::function. I thought since we now allow closures, this would complement them nicely.

I see, is it just lambda support that is making the code less simple? You can use a lambda if it is not capturing, but you'll have to cast it to a function pointer (auto won't work in this case) if you want to bind it into a Closure.

[Dana Jansens]

*puts on base hat*

In terms of Callback vs function differences, some are:

- Callback includes support for our refcounting classes and weak pointers

- Our PostTask/MessageLoop systems are built around Callbacks

- We're currently building thread-safety checks into them such as the ones provided in wtf/ and platform/

- Bind prevents binding lambdas to avoid binding things without them being visible to the Callback.

- Bind forces callers to declare raw pointers as Unretained to make potential crashes clear.

- Callback is going to become move-only (most of the time) allowing thread-safe post-and-destroy behaviour.

There's no document that does this for all of base (or other general containers outside of base) AFAIK. It'd be nice if header files explained these types of things generally. I'll keep this in mind when reviewing patches in the future, it's a good point.

[Jeremy Roman]

Given this (and the similar situation in WTF), perhaps std::bind and std::function should be moved to banned (possibly with exceptions for interfacing with things that don't use base or wtf) with justification? chromium-cpp.appspot.com does seem the correct place for the sort of information pkasting asked for.

--

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/CAHtyhaQEebj_QDLX3kCzmL%2Bri%2B5zxXwLoCfV8GOM%2B7Q%2B-av_Kw%40mail.gmail.com.

[lo...@google.com]

Sorry for rising this issue again.

1) 'Lambda Expressions' section allows _capturing_ lambdas for simple cases except "do not bind or store any capturing lambdas outside the lifetime of the stack frame they are defined in; use base::Callback for this instead"

2) chromium-cpp.appspot.com marks std::function as banned but we already use it implicitly (via auto) in many places:
https://cs.chromium.org/search/?q=%22%3D+%5B%26%22&sq=package:chromium&type=cs

Can we allow std::function or make the documentation+code consistent, at least?

[Peter Kasting]

On Thu, May 10, 2018 at 11:29 PM <lo...@google.com> wrote:

1) 'Lambda Expressions' section allows _capturing_ lambdas for simple cases except "do not bind or store any capturing lambdas outside the lifetime of the stack frame they are defined in; use base::Callback for this instead"

2) chromium-cpp.appspot.com marks std::function as banned but we already use it implicitly (via auto) in many places:
https://cs.chromium.org/search/?q=%22%3D+%5B%26%22&sq=package:chromium&type=cs

The type of an auto used to store a lambda is not std::function; it's an implementation-defined type.  A lambda can be stored in a std::function, but the two aren't the same.

So I'm not seeing what the code/documentation inconsistency is.

PK

[lo...@google.com]

I see.
I should read it as "any callable objects allowed to store a lambda except std::function".
Thanks.

[Yuta Kitamura]

Peter is correct. It returns an unnamed function object, not std::function: http://en.cppreference.com/w/cpp/language/lambda

A lambda closure knows which function to invoke, so it can call the actual function directly, while std::function has to do an indirect (virtual) call. std::function is less efficient in this regard. Thus, it's actually a good idea to *not* convert a lambda closure to std::function if possible, which means you generally *should* capture it as "auto".

auto f = []() { ... };

// ...

f();  // Faster than a call via std::function.

[Peter Kasting]

On Thu, May 10, 2018 at 11:51 PM loyso via cxx <c...@chromium.org> wrote:

I should read it as "any callable objects allowed to store a lambda except std::function".

I'm not sure what the "it" in this sentence is.

Basically, you can bind captureless lambdas for any use, or store them using auto.  You can bind/store capturing lambdas only within a local scope; otherwise use base::Callback.  Never use std::bind or std::function.

PK 

[lo...@google.com]

I think, I have just three choices to implement a visitor pattern and use with capturing lambdas:

Given target.Visit(visitor)
1) Make |Visit| a template function to receive any callable object as an argument
2) Make |Visit| to receive base::Callback as an argument.
3) Write custom iterators and use range-based for loop (no lambdas needed).

[Peter Kasting]

On Fri, May 11, 2018 at 12:13 AM loyso via cxx <c...@chromium.org> wrote:

I think, I have just three choices to implement a visitor pattern and use with capturing lambdas:

Given target.Visit(visitor)
1) Make |Visit| a template function to receive any callable object as an argument
2) Make |Visit| to receive base::Callback as an argument.
3) Write custom iterators and use range-based for loop (no lambdas needed).

Generally, do (2); rarely, do (3).  Don't do (1).

PK

[lo...@chromium.org]

May I ask, why? :)

And why std::function (say, an option 4) is banned to use in this simple case?
 

PK

[Peter Kasting]

On Fri, May 11, 2018 at 12:55 AM <lo...@chromium.org> wrote:

On Friday, May 11, 2018 at 5:46:10 PM UTC+10, Peter Kasting wrote:

On Fri, May 11, 2018 at 12:13 AM loyso via cxx <c...@chromium.org> wrote:

I think, I have just three choices to implement a visitor pattern and use with capturing lambdas:

Given target.Visit(visitor)
1) Make |Visit| a template function to receive any callable object as an argument
2) Make |Visit| to receive base::Callback as an argument.
3) Write custom iterators and use range-based for loop (no lambdas needed).

Generally, do (2); rarely, do (3). 

 

Don't do (1).

May I ask, why? :)

Because we already have Callback for that.  The point of Callback is for people to use it consistently for "callable thing that is passed around".  Even if all other solutions are equal in all other ways (which they're not, see notes on why we prefer Callback over std::function), we should standardize on one pattern, and Callback is that one.

And why std::function (say, an option 4) is banned to use in this simple case?

Again, because we have Callback and should use it in any case where someone would use a std::function.

In a lot of these cases, it doesn't even come down to what the best possible option for some specific scenario is; it comes down to have simple rules like "always use Callback" that are at least good enough all the time.

PK 

[lo...@chromium.org]

Well, using Callback everywhere leads to the following verbosity:

Forced Callback everywhere:

  int unmounted_count = 0;

  DoForEveryUnmountedCryptohome(base::Bind(

      [](int* count, const base::FilePath&) { ++(*count); }, &unmounted_count));

instead of much simpler:

  int unmounted_count = 0;

  DoForEveryUnmountedCryptohome(base::Bind(

      [&unmounted_count](const base::FilePath&) { ++unmounted_count; }));

[lo...@chromium.org]

And with std::function:

  int unmounted_count = 0;

  DoForEveryUnmountedCryptohome([&](const base::FilePath&) { ++unmounted_count; });

[Alex Clarke]

Don't we run the risk of some things accepting std::function and others base::Callback.  That feels undesirable. Also the std::bind machinery is different so presumably this would increase binary bloat.

--

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/e91b9220-b2f0-47ec-bc18-be850075f0dc%40chromium.org.

[Yuta Kitamura]

On Fri, May 11, 2018 at 5:19 PM <lo...@chromium.org> wrote:

Well, using Callback everywhere leads to the following verbosity:

Forced Callback everywhere:

  int unmounted_count = 0;

  DoForEveryUnmountedCryptohome(base::Bind(

      [](int* count, const base::FilePath&) { ++(*count); }, &unmounted_count));

instead of much simpler:

  int unmounted_count = 0;

  DoForEveryUnmountedCryptohome(base::Bind(

      [&unmounted_count](const base::FilePath&) { ++unmounted_count; }));

(I don't think this is very simpler than the above, but)

I think this is OK as long as DoForEveryUnmountedCryptohome doesn't save the passed Callback to anywhere (just call it X times and return). That's clearly true according to the code -- the function name and the use of unmounted_count after the call should be sufficient to prove that.

Or am I misreading the style guide?

 

On Friday, May 11, 2018 at 6:01:31 PM UTC+10, Peter Kasting wrote:

On Fri, May 11, 2018 at 12:55 AM <lo...@chromium.org> wrote:

On Friday, May 11, 2018 at 5:46:10 PM UTC+10, Peter Kasting wrote:

On Fri, May 11, 2018 at 12:13 AM loyso via cxx <c...@chromium.org> wrote:

I think, I have just three choices to implement a visitor pattern and use with capturing lambdas:

Given target.Visit(visitor)
1) Make |Visit| a template function to receive any callable object as an argument
2) Make |Visit| to receive base::Callback as an argument.
3) Write custom iterators and use range-based for loop (no lambdas needed).

Generally, do (2); rarely, do (3). 

 

Don't do (1).

May I ask, why? :)

Because we already have Callback for that.  The point of Callback is for people to use it consistently for "callable thing that is passed around".  Even if all other solutions are equal in all other ways (which they're not, see notes on why we prefer Callback over std::function), we should standardize on one pattern, and Callback is that one.

And why std::function (say, an option 4) is banned to use in this simple case?

Again, because we have Callback and should use it in any case where someone would use a std::function.

In a lot of these cases, it doesn't even come down to what the best possible option for some specific scenario is; it comes down to have simple rules like "always use Callback" that are at least good enough all the time.

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/2c14de8f-5bbb-4d07-a416-41c71fcc79dc%40chromium.org.

[Jeremy Roman]

On Fri, May 11, 2018 at 5:22 AM, Yuta Kitamura <yu...@chromium.org> wrote:

On Fri, May 11, 2018 at 5:19 PM <lo...@chromium.org> wrote:

Well, using Callback everywhere leads to the following verbosity:

Forced Callback everywhere:

  int unmounted_count = 0;

  DoForEveryUnmountedCryptohome(base::Bind(

      [](int* count, const base::FilePath&) { ++(*count); }, &unmounted_count));

instead of much simpler:

  int unmounted_count = 0;

  DoForEveryUnmountedCryptohome(base::Bind(

      [&unmounted_count](const base::FilePath&) { ++unmounted_count; }));

(I don't think this is very simpler than the above, but)

I think this is OK as long as DoForEveryUnmountedCryptohome doesn't save the passed Callback to anywhere (just call it X times and return). That's clearly true according to the code -- the function name and the use of unmounted_count after the call should be sufficient to prove that.

Or am I misreading the style guide?

Using a capturing lambda here is fine because it doesn't escape the local scope, and works if DoForEveryUnmountedCryptohome if a template (which is entirely reasonable in some places, and unreasonable in others). Outside of tests, we don't allow making a base::Callback from a capturing lambda, because we want to provide the guard rails that base::Bind has. We're willing to pay the occasional extra verbosity for catching errors in production code.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAFJcur8mpd3drVSvL3K74dPX1nkhtVQDnaEYkTMV8_2iH9r2Ow%40mail.gmail.com.

[lo...@chromium.org]

On Saturday, May 12, 2018 at 12:14:42 AM UTC+10, Jeremy Roman wrote:

On Fri, May 11, 2018 at 5:22 AM, Yuta Kitamura <yu...@chromium.org> wrote:

On Fri, May 11, 2018 at 5:19 PM <lo...@chromium.org> wrote:

Well, using Callback everywhere leads to the following verbosity:

Forced Callback everywhere:

  int unmounted_count = 0;

  DoForEveryUnmountedCryptohome(base::Bind(

      [](int* count, const base::FilePath&) { ++(*count); }, &unmounted_count));

instead of much simpler:

  int unmounted_count = 0;

  DoForEveryUnmountedCryptohome(base::Bind(

      [&unmounted_count](const base::FilePath&) { ++unmounted_count; }));

(I don't think this is very simpler than the above, but)

I think this is OK as long as DoForEveryUnmountedCryptohome doesn't save the passed Callback to anywhere (just call it X times and return). That's clearly true according to the code -- the function name and the use of unmounted_count after the call should be sufficient to prove that.

Or am I misreading the style guide?

Using a capturing lambda here is fine because it doesn't escape the local scope, and works if DoForEveryUnmountedCryptohome if a template (which is entirely reasonable in some places, and unreasonable in others). Outside of tests, we don't allow making a base::Callback from a capturing lambda, because we want to provide the guard rails that base::Bind has. We're willing to pay the occasional extra verbosity for catching errors in production code.

And by guard rails you also mean an ongoing migration to BindOnce and BindRepeatedly, right?
Will it affect the code in my example? Will we wrap |unmounted_count| into scoped_refptr to bypass raw pointer guards?

[Jeremy Roman]

On Fri, May 11, 2018 at 9:42 PM, <lo...@chromium.org> wrote:

On Saturday, May 12, 2018 at 12:14:42 AM UTC+10, Jeremy Roman wrote:

On Fri, May 11, 2018 at 5:22 AM, Yuta Kitamura <yu...@chromium.org> wrote:

On Fri, May 11, 2018 at 5:19 PM <lo...@chromium.org> wrote:

Well, using Callback everywhere leads to the following verbosity:

Forced Callback everywhere:

  int unmounted_count = 0;

  DoForEveryUnmountedCryptohome(base::Bind(

      [](int* count, const base::FilePath&) { ++(*count); }, &unmounted_count));

instead of much simpler:

  int unmounted_count = 0;

  DoForEveryUnmountedCryptohome(base::Bind(

      [&unmounted_count](const base::FilePath&) { ++unmounted_count; }));

(I don't think this is very simpler than the above, but)

I think this is OK as long as DoForEveryUnmountedCryptohome doesn't save the passed Callback to anywhere (just call it X times and return). That's clearly true according to the code -- the function name and the use of unmounted_count after the call should be sufficient to prove that.

Or am I misreading the style guide?

Using a capturing lambda here is fine because it doesn't escape the local scope, and works if DoForEveryUnmountedCryptohome if a template (which is entirely reasonable in some places, and unreasonable in others). Outside of tests, we don't allow making a base::Callback from a capturing lambda, because we want to provide the guard rails that base::Bind has. We're willing to pay the occasional extra verbosity for catching errors in production code.

And by guard rails you also mean an ongoing migration to BindOnce and BindRepeatedly, right?
Will it affect the code in my example? Will we wrap |unmounted_count| into scoped_refptr to bypass raw pointer guards?

I don't recall exactly what's there today (base/ OWNERS would know more), but we've done things like "require base::Unretained to identify any raw pointers which are being passed but whose lifetime is not assured by the callback". We can't retrofit that onto capturing lambda, so using capture there would mean giving that safety up.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/7b73b3fe-1d46-45a7-8fb8-c65dada66376%40chromium.org.