Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11369061:
Disassociate argument type from return type in PostTaskAndReplyWithResult template. (Closed)
|
Created:
8 years, 1 month ago by awong Modified:
8 years ago CC:
chromium-reviews, erikwright+watch_chromium.org, hashimoto Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDisassociate argument type from return type in PostTaskAndReplyWithResult template.
This is needed to allow the callback system to perform implicit conversions from the task result to the argument's call. Otherwise, the template resolution will force exact equality between the return type of the task and the reply's argument type which is too strict. For example, you wouldn't be able to take a const int & when returning an int.
Also remove some DCHECKs that can't happen.
BUG=159114
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=169825
Patch Set 1 #
Total comments: 12
Patch Set 2 : scrubbed impl. Removed some helper funcs and some overzealous sanity checks. #Patch Set 3 : fix comment #Patch Set 4 : Don't rely on PTARWR allowing a null reply. #
Total comments: 2
Patch Set 5 : Revert is_null() strictness. #
Total comments: 4
Patch Set 6 : Move null check back into ReplyAdapter. #Patch Set 7 : fix cut/paste error #Messages
Total messages: 18 (0 generated)
Thanks! LGTM
https://codereview.chromium.org/11369061/diff/1/base/task_runner_util.h File base/task_runner_util.h (right): https://codereview.chromium.org/11369061/diff/1/base/task_runner_util.h#newco... base/task_runner_util.h:35: template <typename ReturnType, typename ResultType> Could you give these template arguments the same names as you're using in PostTaskAndReplyWithResult? https://codereview.chromium.org/11369061/diff/1/base/task_runner_util.h#newco... base/task_runner_util.h:44: template <typename ReturnType, typename ResultType> Ditto here. https://codereview.chromium.org/11369061/diff/1/base/task_runner_util.h#newco... base/task_runner_util.h:71: const Callback<void(ReplyArgType)>& reply) { Could you double-check that users get a sensible error message when TaskReturnType isn't convertible to ReplyArgType? https://codereview.chromium.org/11369061/diff/1/base/task_runner_util_unittes... File base/task_runner_util_unittest.cc (right): https://codereview.chromium.org/11369061/diff/1/base/task_runner_util_unittes... base/task_runner_util_unittest.cc:82: TEST(TaskRunnerHelpersTest, PostTaskAndReplyWithResultImplitConvert) { sp: Implit https://codereview.chromium.org/11369061/diff/1/base/task_runner_util_unittes... base/task_runner_util_unittest.cc:94: EXPECT_DOUBLE_EQ(42.0f, result); s/42.0f/42.0/ since it's supposed to be a double, not a float.
PTAL https://codereview.chromium.org/11369061/diff/1/base/task_runner_util.h File base/task_runner_util.h (right): https://codereview.chromium.org/11369061/diff/1/base/task_runner_util.h#newco... base/task_runner_util.h:35: template <typename ReturnType, typename ResultType> On 2012/11/05 22:13:22, Jeffrey Yasskin wrote: > Could you give these template arguments the same names as you're using in > PostTaskAndReplyWithResult? Done. https://codereview.chromium.org/11369061/diff/1/base/task_runner_util.h#newco... base/task_runner_util.h:44: template <typename ReturnType, typename ResultType> On 2012/11/05 22:13:22, Jeffrey Yasskin wrote: > Ditto here. Done. https://codereview.chromium.org/11369061/diff/1/base/task_runner_util.h#newco... base/task_runner_util.h:44: template <typename ReturnType, typename ResultType> On 2012/11/05 22:13:22, Jeffrey Yasskin wrote: > Ditto here. Done. https://codereview.chromium.org/11369061/diff/1/base/task_runner_util.h#newco... base/task_runner_util.h:71: const Callback<void(ReplyArgType)>& reply) { On 2012/11/05 22:13:22, Jeffrey Yasskin wrote: > Could you double-check that users get a sensible error message when > TaskReturnType isn't convertible to ReplyArgType? Define sensible :) It looks good enough to me. The compile error happens at the CallbackForward() inside the adapter function below and the conversion shows up first on the error list which is a bonus. In file included from ../../base/task_runner_util_unittest.cc:5: ../../base/task_runner_util.h:30:16: error: reference to type 'const double' could not bind to an lvalue of type 'char *' callback.Run(CallbackForward(*result)); ^~~~~~~~~~~~~~~~~~~~~~~~ ../../base/task_runner_util.h:61:18: note: in instantiation of function template specialization 'base::internal::ReplyAdapter<char *, double>' requested here base::Bind(&internal::ReplyAdapter<TaskReturnType, ReplyArgType>, reply, ^ ../../base/task_runner_util_unittest.cc:90:3: note: in instantiation of function template specialization 'base::PostTaskAndReplyWithResult<char *, double>' requested here PostTaskAndReplyWithResult( ^ ../../base/callback.h:427:65: note: passing argument to parameter 'a1' here R Run(typename internal::CallbackParamTraits<A1>::ForwardType a1) const { ^ 2 errors generated. ninja: build stopped: subcommand failed. https://codereview.chromium.org/11369061/diff/1/base/task_runner_util_unittes... File base/task_runner_util_unittest.cc (right): https://codereview.chromium.org/11369061/diff/1/base/task_runner_util_unittes... base/task_runner_util_unittest.cc:82: TEST(TaskRunnerHelpersTest, PostTaskAndReplyWithResultImplitConvert) { On 2012/11/05 22:13:22, Jeffrey Yasskin wrote: > sp: Implit Done. https://codereview.chromium.org/11369061/diff/1/base/task_runner_util_unittes... base/task_runner_util_unittest.cc:94: EXPECT_DOUBLE_EQ(42.0f, result); On 2012/11/05 22:13:22, Jeffrey Yasskin wrote: > s/42.0f/42.0/ since it's supposed to be a double, not a float. Done.
Apparently someone is relying on the "feature" of accepting |reply| Closures that are null. Suck. On 2012/11/19 05:57:09, awong wrote: > PTAL > > https://codereview.chromium.org/11369061/diff/1/base/task_runner_util.h > File base/task_runner_util.h (right): > > https://codereview.chromium.org/11369061/diff/1/base/task_runner_util.h#newco... > base/task_runner_util.h:35: template <typename ReturnType, typename ResultType> > On 2012/11/05 22:13:22, Jeffrey Yasskin wrote: > > Could you give these template arguments the same names as you're using in > > PostTaskAndReplyWithResult? > > Done. > > https://codereview.chromium.org/11369061/diff/1/base/task_runner_util.h#newco... > base/task_runner_util.h:44: template <typename ReturnType, typename ResultType> > On 2012/11/05 22:13:22, Jeffrey Yasskin wrote: > > Ditto here. > > Done. > > https://codereview.chromium.org/11369061/diff/1/base/task_runner_util.h#newco... > base/task_runner_util.h:44: template <typename ReturnType, typename ResultType> > On 2012/11/05 22:13:22, Jeffrey Yasskin wrote: > > Ditto here. > > Done. > > https://codereview.chromium.org/11369061/diff/1/base/task_runner_util.h#newco... > base/task_runner_util.h:71: const Callback<void(ReplyArgType)>& reply) { > On 2012/11/05 22:13:22, Jeffrey Yasskin wrote: > > Could you double-check that users get a sensible error message when > > TaskReturnType isn't convertible to ReplyArgType? > > Define sensible :) > > It looks good enough to me. The compile error happens at the CallbackForward() > inside the adapter function below and the conversion shows up first on the error > list which is a bonus. > > In file included from ../../base/task_runner_util_unittest.cc:5: > ../../base/task_runner_util.h:30:16: error: reference to type 'const double' > could not bind to an lvalue of type 'char *' > callback.Run(CallbackForward(*result)); > ^~~~~~~~~~~~~~~~~~~~~~~~ > ../../base/task_runner_util.h:61:18: note: in instantiation of function template > specialization 'base::internal::ReplyAdapter<char *, double>' requested here > base::Bind(&internal::ReplyAdapter<TaskReturnType, ReplyArgType>, reply, > ^ > ../../base/task_runner_util_unittest.cc:90:3: note: in instantiation of function > template specialization 'base::PostTaskAndReplyWithResult<char *, double>' > requested here > PostTaskAndReplyWithResult( > ^ > ../../base/callback.h:427:65: note: passing argument to parameter 'a1' here > R Run(typename internal::CallbackParamTraits<A1>::ForwardType a1) const { > ^ > 2 errors generated. > ninja: build stopped: subcommand failed. > > https://codereview.chromium.org/11369061/diff/1/base/task_runner_util_unittes... > File base/task_runner_util_unittest.cc (right): > > https://codereview.chromium.org/11369061/diff/1/base/task_runner_util_unittes... > base/task_runner_util_unittest.cc:82: TEST(TaskRunnerHelpersTest, > PostTaskAndReplyWithResultImplitConvert) { > On 2012/11/05 22:13:22, Jeffrey Yasskin wrote: > > sp: Implit > > Done. > > https://codereview.chromium.org/11369061/diff/1/base/task_runner_util_unittes... > base/task_runner_util_unittest.cc:94: EXPECT_DOUBLE_EQ(42.0f, result); > On 2012/11/05 22:13:22, Jeffrey Yasskin wrote: > > s/42.0f/42.0/ since it's supposed to be a double, not a float. > > Done.
LGTM once you figure out the other places that depend on null callback arguments. http://codereview.chromium.org/11369061/diff/1/base/task_runner_util.h File base/task_runner_util.h (right): http://codereview.chromium.org/11369061/diff/1/base/task_runner_util.h#newcode71 base/task_runner_util.h:71: const Callback<void(ReplyArgType)>& reply) { On 2012/11/19 05:57:09, awong wrote: > On 2012/11/05 22:13:22, Jeffrey Yasskin wrote: > > Could you double-check that users get a sensible error message when > > TaskReturnType isn't convertible to ReplyArgType? > > Define sensible :) > > It looks good enough to me. The compile error happens at the CallbackForward() > inside the adapter function below and the conversion shows up first on the error > list which is a bonus. > > In file included from ../../base/task_runner_util_unittest.cc:5: > ../../base/task_runner_util.h:30:16: error: reference to type 'const double' > could not bind to an lvalue of type 'char *' > callback.Run(CallbackForward(*result)); > ^~~~~~~~~~~~~~~~~~~~~~~~ > ../../base/task_runner_util.h:61:18: note: in instantiation of function template > specialization 'base::internal::ReplyAdapter<char *, double>' requested here > base::Bind(&internal::ReplyAdapter<TaskReturnType, ReplyArgType>, reply, > ^ > ../../base/task_runner_util_unittest.cc:90:3: note: in instantiation of function > template specialization 'base::PostTaskAndReplyWithResult<char *, double>' > requested here > PostTaskAndReplyWithResult( > ^ > ../../base/callback.h:427:65: note: passing argument to parameter 'a1' here > R Run(typename internal::CallbackParamTraits<A1>::ForwardType a1) const { > ^ > 2 errors generated. > ninja: build stopped: subcommand failed. LGTM http://codereview.chromium.org/11369061/diff/10002/base/task_runner_util.h File base/task_runner_util.h (right): http://codereview.chromium.org/11369061/diff/10002/base/task_runner_util.h#ne... base/task_runner_util.h:26: // Adapts a T* result to a calblack that expects a T. sp: calblack
FYI, I had to remove the is_null() DCHECK. Too viral of a change. Instead, I filed a bug for it here: https://code.google.com/p/chromium/issues/detail?id=162712 https://codereview.chromium.org/11369061/diff/10002/base/task_runner_util.h File base/task_runner_util.h (right): https://codereview.chromium.org/11369061/diff/10002/base/task_runner_util.h#n... base/task_runner_util.h:26: // Adapts a T* result to a calblack that expects a T. On 2012/11/19 10:33:46, Jeffrey Yasskin wrote: > sp: calblack Done.
darin: will's out. Mind doing OWNERS?
https://codereview.chromium.org/11369061/diff/18001/base/task_runner_util.h File base/task_runner_util.h (right): https://codereview.chromium.org/11369061/diff/18001/base/task_runner_util.h#n... base/task_runner_util.h:55: if (reply.is_null()) { What is the use case for a null reply? Isn't this just code bloat for most users of PostTaskAndReplyWithResult?
https://codereview.chromium.org/11369061/diff/18001/base/task_runner_util.h File base/task_runner_util.h (right): https://codereview.chromium.org/11369061/diff/18001/base/task_runner_util.h#n... base/task_runner_util.h:55: if (reply.is_null()) { On 2012/11/27 21:26:17, darin wrote: > What is the use case for a null reply? Isn't this just code bloat for most > users of PostTaskAndReplyWithResult? This was behavior that was added during the original implementation (see line 39 on the left file). I tried to remove it, but a lot of code in FileUtil has started depending on it. Here's the bug: https://code.google.com/p/chromium/issues/detail?id=162712 I didn't add a TODO into this because I wanted a second opinion on whether or not we should remove this null check. If you also think it should be gone, then I'll add a TODO and kill it in a followup CL.
https://codereview.chromium.org/11369061/diff/18001/base/task_runner_util.h File base/task_runner_util.h (right): https://codereview.chromium.org/11369061/diff/18001/base/task_runner_util.h#n... base/task_runner_util.h:55: if (reply.is_null()) { On 2012/11/27 21:30:36, awong wrote: > On 2012/11/27 21:26:17, darin wrote: > > What is the use case for a null reply? Isn't this just code bloat for most > > users of PostTaskAndReplyWithResult? > > This was behavior that was added during the original implementation (see line 39 > on the left file). I tried to remove it, but a lot of code in FileUtil has > started depending on it. > > Here's the bug: > https://code.google.com/p/chromium/issues/detail?id=162712 > > I didn't add a TODO into this because I wanted a second opinion on whether or > not we should remove this null check. If you also think it should be gone, then > I'll add a TODO and kill it in a followup CL. Oh, I see. Why not just preserve the null check in ReplyAdapter? I'm thinking about code bloat.
https://codereview.chromium.org/11369061/diff/18001/base/task_runner_util.h File base/task_runner_util.h (right): https://codereview.chromium.org/11369061/diff/18001/base/task_runner_util.h#n... base/task_runner_util.h:55: if (reply.is_null()) { On 2012/11/27 21:35:30, darin wrote: > On 2012/11/27 21:30:36, awong wrote: > > On 2012/11/27 21:26:17, darin wrote: > > > What is the use case for a null reply? Isn't this just code bloat for most > > > users of PostTaskAndReplyWithResult? > > > > This was behavior that was added during the original implementation (see line > 39 > > on the left file). I tried to remove it, but a lot of code in FileUtil has > > started depending on it. > > > > Here's the bug: > > https://code.google.com/p/chromium/issues/detail?id=162712 > > > > I didn't add a TODO into this because I wanted a second opinion on whether or > > not we should remove this null check. If you also think it should be gone, > then > > I'll add a TODO and kill it in a followup CL. > > Oh, I see. Why not just preserve the null check in ReplyAdapter? I'm thinking > about code bloat. Not quite sure I understand. You still have to instantiate the if-statement in both cases. Are you trying to save the extra code for the PostTask() call? Totally willing to do that, but the tradeoff is that null reply users end up with a random extra refcoutned PostTaskAndReplyRelay and one more thread bounce at runtime.
Moved check back up into ReplyAdapter and added TODO() comment referencing bug.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajwong@chromium.org/11369061/23012
Retried try job too often for step(s) aura_unit_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajwong@chromium.org/11369061/23012
Message was sent while issue was closed.
Change committed as 169825 |
