Default behavior of base::OnceClosure()

208 views
Skip to first unread message

Fabio Tirelo

unread,
Jun 15, 2017, 7:12:57 PM6/15/17
to chromi...@chromium.org
Hi all.

In a recent code review, I had a piece of code that looked like this (in that context, we wanted to send a no-op callback as a parameter):

  SomeFunction(..., base::BindOnce([] { /* do nothing */}), ...);

It would be more clear if we could instead use base::OnceClosure(). However, the default constructor creates an unbound callback, which will lead to a crash if we try to run, like in the following toy example:

  auto c = base::OnceClosure();
  std::move(c).Run();

Would it make sense to change the default behavior of base::OnceClosure() to be the same as base::BindOnce([] {})? Another alternative is to create an auxiliary function, maybe in callback_helpers, to return a no-op closure.

Thoughts?

Thanks,

Daniel Cheng

unread,
Jun 15, 2017, 7:52:42 PM6/15/17
to fti...@chromium.org, chromi...@chromium.org
It seems useful to be able to distinguish between an unbound and bound callback. I suppose one possibility would be to use Optional instead, but that would be a pretty major change (and would also differ from the behavior of things like std::function).

As an alternative, there's base::BindOnce(&base::DoNothing). Does that work?

Daniel

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJtUDMwF64o7BiV2g83eEazSqEhkuY2-73NTfLRgtesXFWvaMQ%40mail.gmail.com.

Peter Kasting

unread,
Jun 16, 2017, 3:03:58 AM6/16/17
to Daniel Cheng, fti...@chromium.org, chromi...@chromium.org
On Thu, Jun 15, 2017 at 4:51 PM, Daniel Cheng <dch...@chromium.org> wrote:
It seems useful to be able to distinguish between an unbound and bound callback. I suppose one possibility would be to use Optional instead, but that would be a pretty major change (and would also differ from the behavior of things like std::function).

As an alternative, there's base::BindOnce(&base::DoNothing). Does that work?

I'm confused.  What is an "unbound callback"?  I thought that meant "a callback with some unbound params", but if you just say OnceClosure(), you're not claiming there are any params -- indeed, the definition of a closure is "a callback with no unbound params".  So what's unbound?

I thought it was the case that for things like Closure(), you could create the object and run it.  It seems very surprising if Closure() lets you run it but OnceClosure() does not.

PK

Ken Rockot

unread,
Jun 16, 2017, 3:15:30 AM6/16/17
to Peter Kasting, Daniel Cheng, fti...@chromium.org, chromi...@chromium.org
The word "unbound" here is being used to mean "null."

As dcheng@ says, nullability of callbacks is a useful thing and we rely on it in many places. It would probably be better to use base::Optional now that we have it, but it seems unlikely to be worth the massive churn to change at this point.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

Daniel Cheng

unread,
Jun 16, 2017, 3:16:46 AM6/16/17
to Peter Kasting, fti...@chromium.org, chromi...@chromium.org
Sorry, I was imprecise with my terminology. A default constructed Closure (or OnceClosure) is "null": that is, the functor doesn't point to anything. Calling Run() on a null Closure/OneClosure will crash, rather than silently no-op.

Daniel

Peter Kasting

unread,
Jun 16, 2017, 3:21:40 AM6/16/17
to Daniel Cheng, fti...@chromium.org, chromi...@chromium.org
OK, so that clarifies the terminology.  I'm still unclear on why the default object is not a no-op functor rather than a null functor.  It sounds like you're saying I'm wrong about Closure() doing this.  Does Callback() do this?

Note that on a codereview both thakis and I thought you ought to be able to use the default object like this, so if _none_ of the callback/closure objects let you, that's really surprising.

Also, I'm definitely not proposing eliminating nullable callbacks entirely, just changing what you get when you default-construct one of these.

PK 

Ken Rockot

unread,
Jun 16, 2017, 3:25:09 AM6/16/17
to Peter Kasting, Daniel Cheng, fti...@chromium.org, chromi...@chromium.org
All of our callback types default to a null state.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

Ken Rockot

unread,
Jun 16, 2017, 3:25:55 AM6/16/17
to Peter Kasting, Daniel Cheng, fti...@chromium.org, chromi...@chromium.org
(and you would probably break quite a lot of code in subtle ways if you tried changing that at this point)

Adam Rice

unread,
Jun 16, 2017, 8:00:30 AM6/16/17
to roc...@chromium.org, Peter Kasting, Daniel Cheng, fti...@chromium.org, chromi...@chromium.org
My mental model is that a OnceCallback is like a std::unique_ptr<BoundFunctionState> and a RepeatingCallback is a like a std::scoped_refptr<BoundFunctionState>. From this point of view it's natural and consistent that the default constructor creates a null callback.

Jeremy Roman

unread,
Jun 16, 2017, 11:50:25 AM6/16/17
to Adam Rice, Ken Rockot, Peter Kasting, Daniel Cheng, fti...@chromium.org, chromi...@chromium.org
On Fri, Jun 16, 2017 at 7:58 AM, Adam Rice <ri...@chromium.org> wrote:
My mental model is that a OnceCallback is like a std::unique_ptr<BoundFunctionState> and a RepeatingCallback is a like a std::scoped_refptr<BoundFunctionState>. From this point of view it's natural and consistent that the default constructor creates a null callback.

On 16 June 2017 at 16:25, Ken Rockot <roc...@chromium.org> wrote:
(and you would probably break quite a lot of code in subtle ways if you tried changing that at this point)

On Fri, Jun 16, 2017 at 12:23 AM, Ken Rockot <roc...@chromium.org> wrote:
All of our callback types default to a null state.

On Fri, Jun 16, 2017 at 12:20 AM, Peter Kasting <pkas...@chromium.org> wrote:
On Fri, Jun 16, 2017 at 12:14 AM, Daniel Cheng <dch...@chromium.org> wrote:
On Fri, Jun 16, 2017 at 12:03 AM Peter Kasting <pkas...@chromium.org> wrote:
On Thu, Jun 15, 2017 at 4:51 PM, Daniel Cheng <dch...@chromium.org> wrote:
It seems useful to be able to distinguish between an unbound and bound callback. I suppose one possibility would be to use Optional instead, but that would be a pretty major change (and would also differ from the behavior of things like std::function).

As an alternative, there's base::BindOnce(&base::DoNothing). Does that work?

I'm confused.  What is an "unbound callback"?  I thought that meant "a callback with some unbound params", but if you just say OnceClosure(), you're not claiming there are any params -- indeed, the definition of a closure is "a callback with no unbound params".  So what's unbound?

I thought it was the case that for things like Closure(), you could create the object and run it.  It seems very surprising if Closure() lets you run it but OnceClosure() does not.

PK

Sorry, I was imprecise with my terminology. A default constructed Closure (or OnceClosure) is "null": that is, the functor doesn't point to anything. Calling Run() on a null Closure/OneClosure will crash, rather than silently no-op.

OK, so that clarifies the terminology.  I'm still unclear on why the default object is not a no-op functor rather than a null functor.  It sounds like you're saying I'm wrong about Closure() doing this.  Does Callback() do this?

For any callback with a non-void return value, you have to produce a value from this "no-op functor". Not all types are default-constructible, so what value do you return?

Making things inconsistent between void-return and non-void-return callbacks seems even more confusing to me, which is why the current situation makes sense to me.
 

Peter Kasting

unread,
Jan 30, 2018, 11:24:59 PM1/30/18
to Jeremy Roman, Adam Rice, Ken Rockot, Daniel Cheng, fti...@chromium.org, chromi...@chromium.org
Sorry to resurrect a dead thread, but this bit me again recently.

I wonder if Run() should automatically no-op when called on a null Callback object.  That seems less likely to break things than making the default constructor construct some sort of non-null callback, and it seems better than crashing.

Or is this still a bad idea, maybe in the vein of "don't paper over DCHECK failures with null-checks"?  Is there utility in the fact that Run()ing a null callback will crash, since that points to some kind of real bug in the code, and you should have fixed it to not get into that state?

PK

Gabriel Charette

unread,
Jan 31, 2018, 2:37:45 AM1/31/18
to pkas...@chromium.org, Jeremy Roman, Adam Rice, Ken Rockot, Daniel Cheng, fti...@chromium.org, chromi...@chromium.org

Do we have a history of problems with null Callbacks? Or would you just rather avoid the if non-null check before running in a few places?

We sure need to keep the operator bool() for empty callbacks but I can't immediately think of reasons why no-op when running null callback would be problematic. But I also don't see why the status quo is problematic either.

As for the OP, we have base::DoNothing in bind_helpers.h (if that wasn't obvious, then perhaps it's a documentation update that's needed?)


--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

Peter Kasting

unread,
Jan 31, 2018, 3:00:24 AM1/31/18
to Gabriel Charette, Jeremy Roman, Adam Rice, Ken Rockot, Daniel Cheng, fti...@chromium.org, chromi...@chromium.org
On Tue, Jan 30, 2018 at 11:36 PM, Gabriel Charette <g...@chromium.org> wrote:

Do we have a history of problems with null Callbacks?

Well, I seem to.  Every six months or so I run into crashes the turn out to be because I wrote code passing a default-constructed callback and expecting it to do nothing when run.  I don't know if anyone else has such issues, though.  As mentioned earlier in this thread, I wasn't the only person who thought things would work this way, but I can't speak to how many others actually seem to write code depending on it.

Or would you just rather avoid the if non-null check before running in a few places?

Actually I suspect we could save quite a number if "if (!null)" checks before calling Run()... that might be a good reason to do this.

As for the OP, we have base::DoNothing in bind_helpers.h (if that wasn't obvious, then perhaps it's a documentation update that's needed?)

Sadly DoNothing() only works for Closures; if the signature is anything else, you need to use Bind with a no-op lambda of the right signature.

I wonder if there's a clever way to use variadic templates or something to write a DoNothing that can automatically be passed to functions expecting any signature of Callback.

PK

Colin Blundell

unread,
Jan 31, 2018, 5:36:27 AM1/31/18
to pkas...@chromium.org, Gabriel Charette, Jeremy Roman, Adam Rice, Ken Rockot, Daniel Cheng, fti...@chromium.org, chromi...@chromium.org
On Wed, Jan 31, 2018 at 9:00 AM Peter Kasting <pkas...@chromium.org> wrote:
On Tue, Jan 30, 2018 at 11:36 PM, Gabriel Charette <g...@chromium.org> wrote:

Do we have a history of problems with null Callbacks?

Well, I seem to.  Every six months or so I run into crashes the turn out to be because I wrote code passing a default-constructed callback and expecting it to do nothing when run.  I don't know if anyone else has such issues, though.  As mentioned earlier in this thread, I wasn't the only person who thought things would work this way, but I can't speak to how many others actually seem to write code depending on it.

I have also had the recurring expectation that a default-constructed callback would be a do-nothing callback rather than one that crashed if ran, and have had to have change in response to being "reminded" that that expectation was incorrect :).
 

Or would you just rather avoid the if non-null check before running in a few places?

Actually I suspect we could save quite a number if "if (!null)" checks before calling Run()... that might be a good reason to do this.

My suspicion is the same. Here's a question: Has anyone perceived there to be value in being able to distinguish a not-set callback from a no-op callback? I never have that I can recall.
 

As for the OP, we have base::DoNothing in bind_helpers.h (if that wasn't obvious, then perhaps it's a documentation update that's needed?)

Sadly DoNothing() only works for Closures; if the signature is anything else, you need to use Bind with a no-op lambda of the right signature.

I wonder if there's a clever way to use variadic templates or something to write a DoNothing that can automatically be passed to functions expecting any signature of Callback.

PK

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

Yuta Kitamura

unread,
Jan 31, 2018, 6:18:47 AM1/31/18
to Peter Kasting, Jeremy Roman, Adam Rice, Ken Rockot, Daniel Cheng, fti...@chromium.org, chromi...@chromium.org
Suppose a caller expects the callback makes some side effect. If it (mistakenly) called a null callback and the program continued without crashing, the diagnosis would become harder...

How about adding a "noop_callback" constant that works like nullptr? E.g.

OnceClosure closure = noop_callback;
closure();  // This is safe

Jeremy Roman

unread,
Jan 31, 2018, 10:37:38 AM1/31/18
to Peter Kasting, Gabriel Charette, Adam Rice, Ken Rockot, Daniel Cheng, Fabio Tirelo, chromi...@chromium.org
Yes, this could be done. Though it's still kinda thorny insofar as non-void-returning functions must produce a value.

Something like:

class DoNothing {
 public:
  template <typename... Args>
  operator RepeatingCallback<void(Args...)>() const {
    return BindRepeating([](Args... args) {});
  }
};

// usage:
base::Callback<void(int)> c = DoNothing();

template <typename R>
class DoNothingAndReturn {
 public:
  explicit DoNothingAndReturn(R result) : result_(std::move(result)) {}

  template <typename... Args>
  operator RepeatingCallback<R(Args...)>() const {
    return BindRepeating([](R result, Args... args) -> R { return result; }, std::move(result_));
  }

 private:
  R result_;
};

// usage:
base::Callback<bool(int)> c = DoNothingAndReturn<bool>(true);
 
Or maybe slightly less magical if you'd rather the caller be forced to supply the Args types. (Or slightly more magical if you want to infer R.)

I wouldn't mind something like this if base/ owners thought it was okay, but I'm not sure I'd want it to be the default behavior of callbacks.

PK

Joe Mason

unread,
Jan 31, 2018, 11:18:56 AM1/31/18
to Jeremy Roman, Peter Kasting, Gabriel Charette, Adam Rice, Ken Rockot, Daniel Cheng, Fabio Tirelo, chromi...@chromium.org
Do we commonly rely on the fact that Callback's can be default-constructed? (To include in containers, for instance.) If that's rare, maybe we can remove the default constructor and add one taking a kNullCallback sentinel for when you really want a null callback.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

Sylvain Defresne

unread,
Jan 31, 2018, 11:37:53 AM1/31/18
to joenot...@chromium.org, Jeremy Roman, Peter Kasting, Gabriel Charette, Adam Rice, Ken Rockot, Daniel Cheng, Fabio Tirelo, chromi...@chromium.org
There is base::CallbackList that store an std::list<> of callbacks, but I don't know if this depends on default constructor.

However, removing the default constructor would make it harder to have Objective-C++ instances keep a callback as a member variable (it would require instead having an std::unique_ptr<base::Callback<...>>) as member variables in Objective-C++ instances needs to be default constructible. I don't know whether this is a frequent pattern or not though.
-- Sylvain

Wez

unread,
Jan 31, 2018, 12:45:04 PM1/31/18
to sdef...@chromium.org, joenot...@chromium.org, Jeremy Roman, Peter Kasting, Gabriel Charette, Adam Rice, Ken Rockot, Daniel Cheng, Fabio Tirelo, chromi...@chromium.org
With OnceCallback I think we rely on the callback becoming null'd when Run() as an implicit means of detecting coding errors in which calling code somehow tries to Run() it more-than-once.

I think it's therefore useful to be able to distinguish the two, but having the _default_ be a DoNothing-ish callback seems like it might be reasonable.  jroman's DoNothing template also seems reasonable, though I'm not sure allowing do-nothing Callbacks with non-void return values makes sense?

Ken Rockot

unread,
Jan 31, 2018, 12:53:19 PM1/31/18
to Wez, Sylvain Defresne, Joe Mason, Jeremy Roman, Peter Kasting, Gabriel Charette, Adam Rice, Daniel Cheng, Fabio Tirelo, chromi...@chromium.org
There don't seem to be compelling reasons to go one way or the other, so it's not clear to me why we'd bother changing the behavior now.

Lei Zhang

unread,
Jan 31, 2018, 1:28:50 PM1/31/18
to Jeremy Roman, Peter Kasting, Gabriel Charette, Adam Rice, Ken Rockot, Daniel Cheng, Fabio Tirelo, chromi...@chromium.org
On Wed, Jan 31, 2018 at 7:35 AM, Jeremy Roman <jbr...@chromium.org> wrote:
> On Wed, Jan 31, 2018 at 2:58 AM, Peter Kasting <pkas...@chromium.org>
> wrote:
>> Sadly DoNothing() only works for Closures; if the signature is anything
>> else, you need to use Bind with a no-op lambda of the right signature.
>>
>> I wonder if there's a clever way to use variadic templates or something to
>> write a DoNothing that can automatically be passed to functions expecting
>> any signature of Callback.
>
>
> Yes, this could be done. Though it's still kinda thorny insofar as
> non-void-returning functions must produce a value.
>
> Something like:
>
> class DoNothing {
> public:
> template <typename... Args>
> operator RepeatingCallback<void(Args...)>() const {
> return BindRepeating([](Args... args) {});
> }
> };

I've fiddled with adding:

template <typename T>
BASE_EXPORT inline void DoNothingWithParam(T) {}

on https://chromium-review.googlesource.com/c/chromium/src/+/745381,
but I can't get it link on Windows and never got back to it.

Peter Kasting

unread,
Jan 31, 2018, 4:25:25 PM1/31/18
to Jeremy Roman, Gabriel Charette, Adam Rice, Ken Rockot, Daniel Cheng, Fabio Tirelo, chromi...@chromium.org
On Wed, Jan 31, 2018 at 7:35 AM, Jeremy Roman <jbr...@chromium.org> wrote:
On Wed, Jan 31, 2018 at 2:58 AM, Peter Kasting <pkas...@chromium.org> wrote:
I wonder if there's a clever way to use variadic templates or something to write a DoNothing that can automatically be passed to functions expecting any signature of Callback.

Yes, this could be done. Though it's still kinda thorny insofar as non-void-returning functions must produce a value.

I wouldn't mind something like this if base/ owners thought it was okay, but I'm not sure I'd want it to be the default behavior of callbacks.

I think DoNothing doesn't really make sense conceptually for callbacks that return a value, anyway.   Returning a value is doing something.  And I've never run into a case where it's reasonable to pass a "do nothing" (in concept) callback when it's supposed to return a value.  Whereas "do nothing" is frequently meaningful for "give me a callback that I should notify when X happens", when the caller doesn't care about X happening (e.g. because we're in a test).  Those sorts of callbacks don't return values.

So I think it'd be sufficient to generalize the existing DoNothing (that returns void) in the way you describe above.  Then people who want a no-op callback could at least always bind that regardless of expected args.

This would sidestep the questions over what should happen when running a null callback, which should make it less controversial.  I still think on balance that should no-op rather than crash, but a reasonable person can disagree, and this at least would help :)

I'll see if I can implement this locally.

PK

Daniel Cheng

unread,
Jan 31, 2018, 4:48:02 PM1/31/18
to Peter Kasting, Jeremy Roman, Gabriel Charette, Adam Rice, Ken Rockot, Fabio Tirelo, chromi...@chromium.org
While the behavior of running a null callback can be inconvenient sometimes, on the whole, I'd prefer to retain the current behavior. Making it silently do nothing would make it easy to mask over issues like posting unbound closures to task runners; it's also make it harder to find errors like running a OnceCallback multiple times.

Making DoNothing more extensible seems reasonable to me though.

Daniel

Jeremy Roman

unread,
Jan 31, 2018, 6:20:51 PM1/31/18
to Peter Kasting, Gabriel Charette, Adam Rice, Ken Rockot, Daniel Cheng, Fabio Tirelo, chromi...@chromium.org
On Wed, Jan 31, 2018 at 4:24 PM, Peter Kasting <pkas...@chromium.org> wrote:
On Wed, Jan 31, 2018 at 7:35 AM, Jeremy Roman <jbr...@chromium.org> wrote:
On Wed, Jan 31, 2018 at 2:58 AM, Peter Kasting <pkas...@chromium.org> wrote:
I wonder if there's a clever way to use variadic templates or something to write a DoNothing that can automatically be passed to functions expecting any signature of Callback.

Yes, this could be done. Though it's still kinda thorny insofar as non-void-returning functions must produce a value.

I wouldn't mind something like this if base/ owners thought it was okay, but I'm not sure I'd want it to be the default behavior of callbacks.

I think DoNothing doesn't really make sense conceptually for callbacks that return a value, anyway.   Returning a value is doing something.  And I've never run into a case where it's reasonable to pass a "do nothing" (in concept) callback when it's supposed to return a value.  Whereas "do nothing" is frequently meaningful for "give me a callback that I should notify when X happens", when the caller doesn't care about X happening (e.g. because we're in a test).  Those sorts of callbacks don't return values.

I don't have a specific instance in mind, but I'd imagined callers that returned a boolean or enum to make a policy decision (should the thing abort at this time?) at such a time, which I've seen elsewhere. But I'm quite happy to do the minimum useful thing and add more only if there's enough demand (it's just sugar anyway).

Gabriel Charette

unread,
Feb 1, 2018, 5:58:29 AM2/1/18
to Jeremy Roman, Peter Kasting, Gabriel Charette, Adam Rice, Ken Rockot, Daniel Cheng, Fabio Tirelo, chromi...@chromium.org
+1 to DoNothing that can bind arbitrary args (but still return void)
+1 to Daniel's stance on null callbacks making it easier to catch errors (e.g. running a OnceClosure multiple times).

Taiju Tsuiki

unread,
Feb 1, 2018, 7:43:21 AM2/1/18
to g...@chromium.org, Jeremy Roman, Peter Kasting, Adam Rice, Ken Rockot, Daniel Cheng, Fabio Tirelo, chromi...@chromium.org
As another proposal to DoNothing for multiple arguments.
Can we just add a tag for it and make an special constructor to Callback?

namespace base {


enum noop_callback_t {noop_callback};


class OnceCallback {

public:

 OnceCallback(noop_callback_t);

};


}  // namespace base


base::OnceCallback<void(int, std::string)> cb = base::noop_callback;



2018年2月1日(木) 19:57 Gabriel Charette <g...@chromium.org>:
--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

Gabriel Charette

unread,
Feb 1, 2018, 7:55:59 AM2/1/18
to Taiju Tsuiki, g...@chromium.org, Jeremy Roman, Peter Kasting, Adam Rice, Ken Rockot, Daniel Cheng, Fabio Tirelo, chromi...@chromium.org
That would work.

Would keep OnceClosure becomes null after being ran property.

Default constructor would still be a null callback.

But we have a trivial way to build a noop_callback.

PS: I lost track, what's the use case for no-op callback again?! Should it be considered IsCancelled()? One of the things I've seen DoNothing() used for was to guarantee a wakeup of given task processing unit... if IsCancelled() then that won't work.

Colin Blundell

unread,
Feb 1, 2018, 9:28:02 AM2/1/18
to g...@chromium.org, Taiju Tsuiki, Jeremy Roman, Peter Kasting, Adam Rice, Ken Rockot, Daniel Cheng, Fabio Tirelo, chromi...@chromium.org
On Thu, Feb 1, 2018 at 1:55 PM Gabriel Charette <g...@chromium.org> wrote:
That would work.

Would keep OnceClosure becomes null after being ran property.

Default constructor would still be a null callback.

But we have a trivial way to build a noop_callback.

PS: I lost track, what's the use case for no-op callback again?!

The primary use case that I've seen is for tests to supply a callback that can be run to production code that expects the callback to be non-null. The alternative is to add the conditional in the production code just for the test, which is unappealing.
 

Gabriel Charette

unread,
Feb 1, 2018, 9:34:38 AM2/1/18
to Colin Blundell, g...@chromium.org, Taiju Tsuiki, Jeremy Roman, Peter Kasting, Adam Rice, Ken Rockot, Daniel Cheng, Fabio Tirelo, chromi...@chromium.org
On Thu, Feb 1, 2018 at 3:26 PM Colin Blundell <blun...@chromium.org> wrote:
On Thu, Feb 1, 2018 at 1:55 PM Gabriel Charette <g...@chromium.org> wrote:
That would work.

Would keep OnceClosure becomes null after being ran property.

Default constructor would still be a null callback.

But we have a trivial way to build a noop_callback.

PS: I lost track, what's the use case for no-op callback again?!

The primary use case that I've seen is for tests to supply a callback that can be run to production code that expects the callback to be non-null. The alternative is to add the conditional in the production code just for the test, which is unappealing.

Ah right, that's neat. In that case we would want the noop_callback to have IsCancelled() == true ideally for the scheduler to discard it ASAP (on post).

David Roger

unread,
Feb 2, 2018, 4:11:45 AM2/2/18
to g...@chromium.org, Colin Blundell, Taiju Tsuiki, Jeremy Roman, Peter Kasting, Adam Rice, Ken Rockot, Daniel Cheng, Fabio Tirelo, chromi...@chromium.org
You can use lambda functions:
base::BindOnce([](int){});
This is pretty short to write, although arguably not very readable.

Peter Kasting

unread,
Feb 2, 2018, 1:32:28 PM2/2/18
to Jeremy Roman, Gabriel Charette, Adam Rice, Ken Rockot, Daniel Cheng, Fabio Tirelo, chromi...@chromium.org
On Wed, Jan 31, 2018 at 1:24 PM, Peter Kasting <pkas...@chromium.org> wrote:
I'll see if I can implement this locally.

Update: I'm in the process of implementing the version that looks like this:

Old: base::Bind(&base::DoNothing)
New: base::DoNothing()

Along the way I'm also cleaning up cases where people manually supplied no-op functions whose names began with "DoNothing", "Noop", or "Empty", as well as cases where people used lambdas as do-nothings.  This won't catch every single convertible instance in the codebase, but it will catch most of them.  In all this will eliminate several hundred lines of code from the codebase in the form of definitions of one-off empty functions.

I'm not implementing a version that returns a value.  I ran into a couple cases of these in test code, but they're rare, and I left them as-is.

It turns out to be useful (albeit rarely) to have explicit DoNothing::Once() and DoNothing::Repeatedly() functions for when you need to force the compiler to do something specific (usually in these cases you end up specifying the args as well, a la DoNothing::Repeatedly<bool>()).  So I'm adding those.  This is your chance to bikeshed whether it should be DoNothing::Repeating() (matches callback name) or DoNothing::Repeatedly() (reads better as English).

I have the "chrome" target compiling locally on Windows and am moving on to the "all" target.  Then other platforms.  Will go up for review soon.

The main roadblock here is actually IWYU failures; because I had to reorganize the contents of bind*.h, I'm having to add #includes of either bind_helpers.h or callback.h to a lot of files that in principle should have already had them.  It would be really awesome if we had some kind of automated IWYU tooling now that we build all platforms with clang...

PK

Daniel Cheng

unread,
Feb 2, 2018, 1:40:13 PM2/2/18
to Peter Kasting, Jeremy Roman, Gabriel Charette, Adam Rice, Ken Rockot, Fabio Tirelo, chromi...@chromium.org
We've tried to get IWYU working multiple times, but it is difficult for various reasons. My workaround was to write a local CL that bruteforces this process: https://codereview.chromium.org/1505823003/. It attempts to build 'all', and any time it encounters an error, it assume it's due to a missing #include and adds it to the first file with an error, before resuming the build process. Rinse and repeat until everything builds (or it gets stuck in an infinite loop).

Granted, this heuristic isn't /always/ correct but it works well enough in practice.

Daniel

Peter Kasting

unread,
Feb 23, 2018, 7:14:14 PM2/23/18
to Jeremy Roman, Gabriel Charette, Adam Rice, Ken Rockot, Daniel Cheng, Fabio Tirelo, chromi...@chromium.org
On Fri, Feb 2, 2018 at 10:30 AM, Peter Kasting <pkas...@chromium.org> wrote:
On Wed, Jan 31, 2018 at 1:24 PM, Peter Kasting <pkas...@chromium.org> wrote:
I'll see if I can implement this locally.

Update: I'm in the process of implementing the version that looks like this:

Old: base::Bind(&base::DoNothing)
New: base::DoNothing()

After far too much of my life, this has now landed.

Assuming it sticks, you can now just pass base::DoNothing() as an argument in any place would previously have done base::Bind(&base::DoNothing) or bound to a noop lambda or function which takes non-void args.

PK

Avi Drissman

unread,
Feb 23, 2018, 9:51:31 PM2/23/18
to Peter Kasting, Jeremy Roman, Gabriel Charette, Adam Rice, Ken Rockot, Daniel Cheng, Fabio Tirelo, chromi...@chromium.org
Woot! Awesome.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
Reply all
Reply to author
Forward
0 new messages