Is OnceCallback intended for use yet?

[Avi Drissman]

I got a review comment that one of my callbacks should be a OnceCallback. I tried switching over, and keep hitting absurdly long errors that make no sense (apparently you can't use base::Passed with a OnceCallback?) as well as fundamental issues like the fact it seems that you can't PostTask a OnceCallback.

I've been grinding away for about an hour slowly figuring this out and getting there, but this is a terrible experience. Is there anything we can do to make this less awful?

Avi

[Nico Weber]

--
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/CACWgwAaWy-g_jinASRdr%2BBat4OnVV4hmKow5xmBfZfOPu0A3tg%40mail.gmail.com.

[Avi Drissman]

As an exercise, start a branch in Chromium, patch in PS2 from https://codereview.chromium.org/2577273002/ , and attempt to turn IconLoader::IconLoadedCallback and IconManager::IconRequestCallback into OnceCallbacks.

If you compile it, you find a series of issues that are easily run across and incredibly difficult to make sense of. If you fail to std::move() the callback before you .Run() it, the error message makes no sense and it is extremely difficult to figure out what's wrong. If you accidentally have a constant callback, the error message makes no sense and it is extremely difficult to figure out what's wrong. If you use base::Passed rather than std::move in base::BindOnce, the error message makes no sense and it is extremely difficult to figure out what's wrong.

My line to wrap a OnceCallback with another OnceCallback in IconManager::LoadIcon now looks like:

  IconRequestCallback callback_runner = base::BindOnce(

      &RunCallbackIfNotCanceled, is_canceled, std::move(callback));

Does that look reasonable? The error message you get when trying to compile it is incomprehensible:

../../base/bind_internal.h:478:9: error: no matching constructor for initialization of 'std::tuple<Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating>, Callback<void (Image *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once> >'

        bound_args_(std::forward<ForwardBoundArgs>(bound_args)...) {

        ^           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

../../base/bind_internal.h:444:9: note: in instantiation of function template specialization 'base::internal::BindState<void (*)(const base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &, base::Callback<void (gfx::Image *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>, gfx::Image *), base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating>, base::Callback<void (gfx::Image *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once> >::BindState<void (*)(const base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &, base::Callback<void (gfx::Image *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>, gfx::Image *), base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &, const base::Callback<void (gfx::Image *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once> >' requested here

      : BindState(IsCancellable{},

        ^

../../base/bind.h:43:27: note: in instantiation of function template specialization 'base::internal::BindState<void (*)(const base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &, base::Callback<void (gfx::Image *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>, gfx::Image *), base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating>, base::Callback<void (gfx::Image *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once> >::BindState<void (*)(const base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &, base::Callback<void (gfx::Image *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>, gfx::Image *), base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &, const base::Callback<void (gfx::Image *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once> >' requested here

  return CallbackType(new BindState(

                          ^

../../chrome/browser/icon_manager.cc:69:47: note: in instantiation of function template specialization 'base::BindOnce<void (*)(const base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &, base::Callback<void (gfx::Image *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>, gfx::Image *), base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &, const base::Callback<void (gfx::Image *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once> >' requested here

  IconRequestCallback callback_runner = base::BindOnce(

                                              ^

/Volumes/src/chrome-git/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/tuple:624:9: note: candidate template ignored: disabled by 'enable_if' [with _AllocArgT = base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating>, _Alloc = base::Callback<void (gfx::Image *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>, _Dummy = true]

        __lazy_and<

        ^

/Volumes/src/chrome-git/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/tuple:639:26: note: candidate template ignored: disabled by 'enable_if' [with _Dummy = true]

                         _CheckArgsConstructor<

                         ^

/Volumes/src/chrome-git/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/tuple:657:26: note: candidate template ignored: disabled by 'enable_if' [with _Dummy = true]

                         _CheckArgsConstructor<

                         ^

/Volumes/src/chrome-git/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/tuple:715:26: note: candidate template ignored: disabled by 'enable_if' [with _Up = <base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &, const base::Callback<void (gfx::Image *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>>, _PackIsTuple = false]

                         _CheckArgsConstructor<

                         ^

/Volumes/src/chrome-git/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/tuple:747:26: note: candidate template ignored: disabled by 'enable_if' [with _Up = <base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &, const base::Callback<void (gfx::Image *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>>]

                         _CheckArgsConstructor<

                         ^

/Volumes/src/chrome-git/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/tuple:780:26: note: candidate template ignored: disabled by 'enable_if' [with _Alloc = base::Callback<void (gfx::Image *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>, _Up = <>]

                         _CheckArgsConstructor<

                         ^

/Volumes/src/chrome-git/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/tuple:799:26: note: candidate template ignored: disabled by 'enable_if' [with _Alloc = base::Callback<void (gfx::Image *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>, _Up = <>]

                         _CheckArgsConstructor<

                         ^

/Volumes/src/chrome-git/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/tuple:617:23: note: candidate constructor template not viable: requires 0 arguments, but 2 were provided

    _LIBCPP_CONSTEXPR tuple()

                      ^

/Volumes/src/chrome-git/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/tuple:682:7: note: candidate constructor template not viable: requires 4 arguments, but 2 were provided

      tuple(allocator_arg_t, const _Alloc& __a, const _Tp& ... __t)

      ^

/Volumes/src/chrome-git/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/tuple:702:7: note: candidate constructor template not viable: requires 4 arguments, but 2 were provided

      tuple(allocator_arg_t, const _Alloc& __a, const _Tp& ... __t)

      ^

/Volumes/src/chrome-git/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/tuple:827:9: note: candidate constructor template not viable: requires single argument '__t', but 2 arguments were provided

        tuple(_Tuple&& __t) _NOEXCEPT_((is_nothrow_constructible<base, _Tuple>::value))

        ^

/Volumes/src/chrome-git/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/tuple:842:9: note: candidate constructor template not viable: requires single argument '__t', but 2 arguments were provided

        tuple(_Tuple&& __t) _NOEXCEPT_((is_nothrow_constructible<base, _Tuple>::value))

        ^

/Volumes/src/chrome-git/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/tuple:855:9: note: candidate constructor template not viable: requires 3 arguments, but 2 were provided

        tuple(allocator_arg_t, const _Alloc& __a, _Tuple&& __t)

        ^

/Volumes/src/chrome-git/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/tuple:869:9: note: candidate constructor template not viable: requires 3 arguments, but 2 were provided

        tuple(allocator_arg_t, const _Alloc& __a, _Tuple&& __t)

        ^

/Volumes/src/chrome-git/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/tuple:620:5: note: candidate constructor not viable: requires 1 argument, but 2 were provided

    tuple(tuple const&) = default;

    ^

/Volumes/src/chrome-git/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/tuple:621:5: note: candidate constructor not viable: requires 1 argument, but 2 were provided

    tuple(tuple&&) = default;

    ^

1 error generated.

I'm not asking for help here; I am abandoning this CL and telling my reviewer that I refuse to use OnceCallback because I am simply not smart enough to make it work. The problem is that I watch CppCon videos. I keep a copy of the C++ spec around to language lawyer people into submission. If I can't make it work, who can?

Avi

[Scott Hess]

On Fri, Dec 16, 2016 at 1:10 PM, Avi Drissman <a...@chromium.org> wrote:
> If you use base::Passed rather than
> std::move in base::BindOnce, the error message makes no sense and it is
> extremely difficult to figure out what's wrong.

Ignoring all the rest, because C++ and templates are hard.

base::Passed() optimizes to move the callback's storage for a
parameter to the target function's parameter. std::move() optimizes
to move your existing storage for that parameter to the callback's
storage. Together you pass your existing storage all the way down to
the target function's parameter. With OnceCallback, the callback
knows it will fire zero or one times, so it doesn't need
base::Passed() to indicate moving storage to the target function's
parameter. std::move() is still useful to move your existing storage
for that parameter into the callback.

Anyhow, there isn't a "rather than", here.

[Maybe it would make sense to simply allow base::Passed() to
base::BindOnce(), as a no-op, and then later when everything is
segregated into once-vs-multiple buckets, most of the base::Passed()
cases can be removed?]

-scott

[Jeremy Roman]

I concede that template error messages can be cryptic.

From my perspective, the root issue here is that PostTask doesn't take a OnceClosure. If it did, this should work fine. base doesn't seem to currently provide an easy way to use a OnceCallback as the functor for a Callback (and doing so is unsafe, though I guess no more so than cases where base::Passed is used).

Here's one way to make it work (by writing that trampoline by hand): https://codereview.chromium.org/2586003002

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACWgwAYLV%2BmxxpF%3Dd-xUa0bW%2BEz8ecNa_kkGNHfvgV7AaYLZhw%40mail.gmail.com.

[Avi Drissman]

Ah, so my inscrutable error was probably that I missed one spot where I was being passed a Callback by const& and thus it was unexpectedly const.

The problem that I see is that we're mixing the template errors that occur when you have move-only types with the template errors that occur when you have binding and forwarding magic.

Sigh.

Avi

[Daniel Cheng]

I would love people to try OnceCallback to help flush out issues like this, but I'm a bit hesitant to promote wider use until we resolve issues like https://crbug.com/668014. We also need to fix PostTask not accepting a OnceCallback (filed https://crbug.com/675327).

As for the compile errors, we tried to improve some of them with CLs like https://codereview.chromium.org/2517793002. I think we should be able to improve the errors for the various cases you hit as well (filed https://crbug.com/675328).

Daniel