Improve compile errors for OnceCallback |
||
Issue descriptionThe compile errors are very hard to parse atm. At minimum, we should improve: - Accidentally trying to use OnceCallback's copy constructor. - Trying to use base::Passed() with BindOnce() - Trying to convert a OnceCallback to a Callback avi@, if you can think of any other useful ones that I forgot to capture, please add them on =)
,
Dec 17 2016
I think that I was accidentally trying to bind a const OnceCallback. I couldn't make any sense of the error message, and it seemed to point at the base::Passed code, so I switched to std::move and the error message changed. I think that I came away with the impression that I couldn't use Passed rather than the correct impression that I'd changed an unimportant aspect and missed the key point of what I was doing wrong.
,
Dec 17 2016
Hmmm, I see. I'm not as inclined to improve the errors for Passed() here then, since I think the long-term plan should be to eventually remove Passed() in favor of std::move(): with distinct callbacks types, we won't need the subtle distinction between Passed and std::move. We should still improve the other errors, and I'm poking at it to try to find a nice way to implement it atm.
,
Dec 18 2016
SGTM. The one I kept hitting was having a const callback and not even realizing it. Callbacks are passed around as const& a lot, and that kills us here.
,
Dec 18 2016
CL @ https://codereview.chromium.org/2581943005/. avi@, if you happen to have the branch with horrific compiler errors still lying around, I'd appreciate it if you could patch it in and see if it improves things at all. If not, that's fine too... it improves (mostly) the cases I tested, but I also wasn't able to improve things as much as I would like: - static_assert for more descriptive errors isn't an option, since that makes type traits return incorrect answers. - deleting copy constructors in the base class results in less readable errors. However, deleting them in the Callback template directly is hard: copy constructors have a very specific signature that cannot have templated types, so a deleted copy constructor can't be conditionally enabled with std::enable_if. - even if I wanted to use a static_assert to improve the copy construction errors, I can't, because there's no templated parameter to delay evaluation of the static_assert. - this can be worked around using partial specializations, but then I have to duplicate the declarations in the base Callback template into the Callback specialization for OnceCallbacks. Ick. In the end, I'm hoping that marking something as explicitly deleted should be idiomatic enough for developers.
,
Dec 19 2016
I'm quite used to the "can't copy because the copy constructor is implicitly deleted" diagnostics, so that's not my problem. My problem is spews like the one I posted at https://groups.google.com/a/chromium.org/d/msg/cxx/WOODFJjIOi0/HKlvV7UjAAAJ . I think that was probably due to trying to bind a const Callback, but I don't know for sure. When I see something like that I either start changing things randomly hoping for things to get better, or just give up.
,
Dec 19 2016
I'll try to replicate the code that led to that compiler spew.
I noticed you mentioned one other thing in this email:
> 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.
But if I try this, I get:
In file included from ../../base/bind_unittest.cc:5:
In file included from ../../base/bind.h:8:
In file included from ../../base/bind_internal.h:13:
In file included from ../../base/bind_helpers.h:168:
../../base/callback.h:55:5: error: static_assert failed "OnceCallback::Run() may only be invoked on an rvalue, i.e. std::move(callback).Run()."
static_assert(!IsOnceCallback<CallbackType>::value,
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../base/bind_unittest.cc:1425:6: note: in instantiation of member function 'base::internal::RunMixin<base::Callback<void (int *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once> >::Run' requested here
cb.Run(nullptr);
Which actually seems pretty good?
,
Dec 19 2016
OK, found another snippet that generates a fair amount of error text:
diff --git a/base/bind_unittest.cc b/base/bind_unittest.cc
index 51b1ed1..8283b4d 100644
--- a/base/bind_unittest.cc
+++ b/base/bind_unittest.cc
@@ -1419,5 +1419,16 @@ TEST(BindDeathTest, NullCallback) {
EXPECT_DCHECK_DEATH(base::Bind(null_cb, 42));
}
+using MyCallback = OnceCallback<void(int*)>;
+void MyCallbackHelper(const Callback<bool()>&,
+ const Callback<void(int*)>&) {
+}
+
+TEST_F(BindTest, CompileErrors) {
+ Callback<bool()> is_cancellable;
+ MyCallback finished_callback;
+ MyCallback cb = BindOnce(&MyCallbackHelper, is_cancellable, std::move(finished_callback));
+}
+
} // namespace
} // namespace base
The errors do point at the right thing after marking a few things as explicitly deleted, but it's not ideal. It generates two errors, instead of one, and the second error only comes after a fair amount of template spew:
../../base/bind_unittest.cc:1430:14: error: conversion function from 'OnceCallback<MakeUnboundRunType<void (*)(const base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &, const base::Callback<void (int *), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &), base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &, base::Callback<void (int *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once> >>' to 'OnceCallback<void (int *)>' invokes a deleted function
MyCallback cb = BindOnce(&MyCallbackHelper, is_cancellable, std::move(finished_callback));
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../base/callback.h:121:3: note: 'Callback<base::Callback<void (), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>, void>'has been explicitly marked deleted here
Callback(const OtherCallback&) = delete;
^
In file included from ../../base/bind_unittest.cc:5:
In file included from ../../base/bind.h:8:
../../base/bind_internal.h:164:21: error: conversion function from 'Callback<[...], 0, 0>' to 'const Callback<[...], 1, 1>' invokes a deleted function
return function(std::forward<RunArgs>(args)...);
^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../base/bind_internal.h:285:20: note: in instantiation of function template specialization 'base::internal::FunctorTraits<void (*)(const base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &, const base::Callback<void (int *), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &), void>::Invoke<base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating>, base::Callback<void (int *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once> >' requested here
return Traits::Invoke(std::forward<Functor>(functor),
^
../../base/bind_internal.h:361:43: note: in instantiation of function template specialization 'base::internal::InvokeHelper<false, void>::MakeItSo<void (*)(const base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &, const base::Callback<void (int *), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &), base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating>, base::Callback<void (int *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once> >' requested here
return InvokeHelper<is_weak_call, R>::MakeItSo(
^
../../base/bind_internal.h:326:12: note: in instantiation of function template specialization 'base::internal::Invoker<base::internal::BindState<void (*)(const base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &, const base::Callback<void (int *), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &), base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating>, base::Callback<void (int *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once> >, void ()>::RunImpl<void (*)(const base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &, const base::Callback<void (int *), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &), std::tuple<base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating>, base::Callback<void (int *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once> >, 0, 1>' requested here
return RunImpl(std::move(storage->functor_),
^
../../base/bind.h:40:45: note: in instantiation of member function 'base::internal::Invoker<base::internal::BindState<void (*)(const base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &, const base::Callback<void (int *), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &), base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating>, base::Callback<void (int *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once> >, void ()>::RunOnce' requested here
PolymorphicInvoke invoke_func = &Invoker::RunOnce;
^
../../base/bind_unittest.cc:1430:19: note: in instantiation of function template specialization 'base::BindOnce<void (*)(const base::Callback<bool(), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &, const base::Callback<void (int *), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &), base::Callback<bool (), base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &, base::Callback<void (int *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once> >' requested here
MyCallback cb = BindOnce(&MyCallbackHelper, is_cancellable, std::move(finished_callback));
^
../../base/callback.h:121:3: note: 'Callback<base::Callback<void (int *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>, void>' has been explicitly marked deleted here
Callback(const OtherCallback&) = delete;
^
2 errors generated.
Ideally, we could add some sort of static assert at a much higher level (inside BindOnce/Bind perhaps?) that would fail-fast if the args can't be properly forwarded.
Another open question is how much the nicer errors will cost us in terms of compile time. Most previous changes have been on rarely instantiated templates, but BindOnce / Bind are instantiated a lot.
,
Dec 19 2016
Re comment 7, yes, I'm getting that assert, and that is helpful. OTOH, it doesn't show up when you try to .Run() a OnceCallback that's actually a const&. That wasn't the really bad error, tho. Lemme get that one for you.
,
Dec 19 2016
Here is a patch to apply; I made it against b43d478991126401b0a03ef49438fcdcda961938. It spews an error for each of the BindOnce calls in IconManager::LoadIcon, and it's not obvious at all from the errors what's wrong or how to deal with it. (Same error for each one.)
,
Dec 19 2016
Re comment 10, spoiler alert! The solution is to *both* fix the pass-by-ref of the callback to IconManager::LoadIcon to be a pass-by-value, *and* to fix the uses of the callbacks within that function to use std::move(). Otherwise you get the error spew.
,
Dec 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3f2fd66ee3c869dbf43bf3c44877406ecfd871f3 commit 3f2fd66ee3c869dbf43bf3c44877406ecfd871f3 Author: dcheng <dcheng@chromium.org> Date: Tue Dec 20 23:56:59 2016 Mark OnceCallback::Run() & const so it matches a const callback object. BUG=675328 Review-Url: https://codereview.chromium.org/2583343002 Cr-Commit-Position: refs/heads/master@{#439938} [modify] https://crrev.com/3f2fd66ee3c869dbf43bf3c44877406ecfd871f3/base/bind_unittest.nc [modify] https://crrev.com/3f2fd66ee3c869dbf43bf3c44877406ecfd871f3/base/callback.h |
||
►
Sign in to add a comment |
||
Comment 1 by dcheng@chromium.org
, Dec 17 2016Btw, I'm not able to replicate a compile error with Passed and BinceOnce. If someone has a snippet that triggers this, please post it here. diff --git a/base/bind_unittest.cc b/base/bind_unittest.cc index c6b53d5..3f5eb0d 100644 --- a/base/bind_unittest.cc +++ b/base/bind_unittest.cc @@ -1415,5 +1415,10 @@ TEST(BindDeathTest, NullCallback) { EXPECT_DCHECK_DEATH(base::Bind(null_cb, 42)); } +TEST(BindTest, CompileErrors) { + std::unique_ptr<int> x; + auto cb = BindOnce([] (std::unique_ptr<int> x) { }, Passed(&x)); +} + } // namespace } // namespace base is what I tried.