Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1512833002:
Remove unbound base::Bind overload (Closed)
DescriptionRemove unbound base::Bind overload
This CL removes a base::Bind overload without bound arguments.
The difference to generic base::Bind is the type check for the bound
Runnable: the generic one ensures any argument of the Runnable
is not non-const reference, OTHT, the overload being removed checks
nothing.
Where, comments and tests asserts any *bound* argument should not be
non-const reference, so the generic one has been checked excessively.
This CL loosen the type check to match the requirement, and then
the overloaded base::Bind is no longer needed.
BUG=
Committed: https://crrev.com/7fe3a687ae2cd98c85e4556441b94e2268704e71
Cr-Commit-Position: refs/heads/master@{#365988}
Patch Set 1 #Patch Set 2 : split out TypeList utilities to separate CLs #Patch Set 3 : merge back #
Total comments: 8
Patch Set 4 : #Patch Set 5 : rebase #
Total comments: 2
Patch Set 6 : +test #Patch Set 7 : fix #
Total comments: 7
Messages
Total messages: 34 (14 generated)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1512833002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1512833002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tzik@chromium.org changed reviewers: + dcheng@chromium.org
PTAL
Can we split TakeTypeListItem and ExtractArgs into a separate CL? I think it's OK to remove this overload without adding that, and it'll separate the simple cleanups from the more complicated new stuff.
Description was changed from ========== Remove unbound base::Bind overload This CL removes a base::Bind overload without bound arguments. The difference to generic base::Bind is the type check for the bound Runnable: the generic one ensures any argument of the bound Runnable is not non-const reference, OTHT, the overload being removed checks nothing. Where, comments and tests asserts any *bound* argument should not be non-const reference, so the generic one has been checked excessively. This CL loosen the type check to match the requirement, and then the overloaded base::Bind is no longer needed. Also adds below as a building block: * TakeTypeListItem, that removes N items from a TypeList. * ExtractArgs that extracts arguments from a function type. BUG= ========== to ========== Remove unbound base::Bind overload This CL removes a base::Bind overload without bound arguments. The difference to generic base::Bind is the type check for the bound Runnable: the generic one ensures any argument of the bound Runnable is not non-const reference, OTHT, the overload being removed checks nothing. Where, comments and tests asserts any *bound* argument should not be non-const reference, so the generic one has been checked excessively. This CL loosen the type check to match the requirement, and then the overloaded base::Bind is no longer needed. BUG= ==========
On 2015/12/09 23:49:30, dcheng wrote: > Can we split TakeTypeListItem and ExtractArgs into a separate CL? I think it's > OK to remove this overload without adding that, and it'll separate the simple > cleanups from the more complicated new stuff. Per local discussion, I combined utilities CLs to avoid adding no-user stuffs.
https://codereview.chromium.org/1512833002/diff/40001/base/bind_helpers.h File base/bind_helpers.h (right): https://codereview.chromium.org/1512833002/diff/40001/base/bind_helpers.h#new... base/bind_helpers.h:541: typedef TypeList<Accum...> Type; Let's do using Type = TypeList<Accum...>; https://codereview.chromium.org/1512833002/diff/40001/base/bind_helpers.h#new... base/bind_helpers.h:549: // A type-level function that takes first |n| list item from given TypeList. Can this comment include an example input and the output? e.g. TakeTypeListItem<3, TypeList<A, B, C, D>> will evaluate to TypeList<A, B, C> https://codereview.chromium.org/1512833002/diff/40001/base/bind_helpers.h#new... base/bind_helpers.h:589: // A type-level function that extracts function arguments into a TypeList. Ditto: Input: R(A1, A2, A3) => TypeList<A1, A2, A3> https://codereview.chromium.org/1512833002/diff/40001/base/bind_internal.h File base/bind_internal.h (left): https://codereview.chromium.org/1512833002/diff/40001/base/bind_internal.h#ol... base/bind_internal.h:81: struct HasNonConstReferenceParam<R(T, Args...)> I have a dumb question: why can't we just add a template specialization for R() with no args here?
https://codereview.chromium.org/1512833002/diff/40001/base/bind_helpers.h File base/bind_helpers.h (right): https://codereview.chromium.org/1512833002/diff/40001/base/bind_helpers.h#new... base/bind_helpers.h:541: typedef TypeList<Accum...> Type; On 2015/12/14 19:26:46, dcheng wrote: > Let's do using Type = TypeList<Accum...>; Done. https://codereview.chromium.org/1512833002/diff/40001/base/bind_helpers.h#new... base/bind_helpers.h:549: // A type-level function that takes first |n| list item from given TypeList. On 2015/12/14 19:26:46, dcheng wrote: > Can this comment include an example input and the output? e.g. > > TakeTypeListItem<3, TypeList<A, B, C, D>> will evaluate to TypeList<A, B, C> Done. https://codereview.chromium.org/1512833002/diff/40001/base/bind_helpers.h#new... base/bind_helpers.h:589: // A type-level function that extracts function arguments into a TypeList. On 2015/12/14 19:26:46, dcheng wrote: > Ditto: > > Input: R(A1, A2, A3) => TypeList<A1, A2, A3> Done. https://codereview.chromium.org/1512833002/diff/40001/base/bind_internal.h File base/bind_internal.h (left): https://codereview.chromium.org/1512833002/diff/40001/base/bind_internal.h#ol... base/bind_internal.h:81: struct HasNonConstReferenceParam<R(T, Args...)> On 2015/12/14 19:26:46, dcheng wrote: > I have a dumb question: why can't we just add a template specialization for R() > with no args here? Add specialization for R() and remove the base case impl? It's a possible option to add it for readability. Using the base case as the "else case" is just a shorthand for fewer specialization.
https://codereview.chromium.org/1512833002/diff/80001/base/bind.h File base/bind.h (right): https://codereview.chromium.org/1512833002/diff/80001/base/bind.h#newcode69 base/bind.h:69: internal::ExtractArgs<BoundRunType>>; OK, I understand why this is necessary now. Should we add another test case to bind_unittest.cc in UnboundArgumentTypeSupport? Given this: void F1(int&); void F2(int, int&); This worked: CallbacK<void(int&)> cv = base::Bind(&F1); But this didn't: Callback<void(int&)> cb = base::Bind(&F2, 0); but with your patch, they both work now.
https://codereview.chromium.org/1512833002/diff/80001/base/bind.h File base/bind.h (right): https://codereview.chromium.org/1512833002/diff/80001/base/bind.h#newcode69 base/bind.h:69: internal::ExtractArgs<BoundRunType>>; On 2015/12/15 19:42:54, dcheng wrote: > OK, I understand why this is necessary now. > > Should we add another test case to bind_unittest.cc in > UnboundArgumentTypeSupport? > > Given this: > void F1(int&); > void F2(int, int&); > > This worked: > CallbacK<void(int&)> cv = base::Bind(&F1); > > But this didn't: > Callback<void(int&)> cb = base::Bind(&F2, 0); > > but with your patch, they both work now. Added. Yes, we should.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1512833002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1512833002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1512833002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1512833002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Remove unbound base::Bind overload This CL removes a base::Bind overload without bound arguments. The difference to generic base::Bind is the type check for the bound Runnable: the generic one ensures any argument of the bound Runnable is not non-const reference, OTHT, the overload being removed checks nothing. Where, comments and tests asserts any *bound* argument should not be non-const reference, so the generic one has been checked excessively. This CL loosen the type check to match the requirement, and then the overloaded base::Bind is no longer needed. BUG= ========== to ========== Remove unbound base::Bind overload This CL removes a base::Bind overload without bound arguments. The difference to generic base::Bind is the type check for the bound Runnable: the generic one ensures any argument of the Runnable is not non-const reference, OTHT, the overload being removed checks nothing. Where, comments and tests asserts any *bound* argument should not be non-const reference, so the generic one has been checked excessively. This CL loosen the type check to match the requirement, and then the overloaded base::Bind is no longer needed. BUG= ==========
Description was changed from ========== Remove unbound base::Bind overload This CL removes a base::Bind overload without bound arguments. The difference to generic base::Bind is the type check for the bound Runnable: the generic one ensures any argument of the Runnable is not non-const reference, OTHT, the overload being removed checks nothing. Where, comments and tests asserts any *bound* argument should not be non-const reference, so the generic one has been checked excessively. This CL loosen the type check to match the requirement, and then the overloaded base::Bind is no longer needed. BUG= ========== to ========== Remove unbound base::Bind overload This CL removes a base::Bind overload without bound arguments. The difference to generic base::Bind is the type check for the bound Runnable: the generic one ensures any argument of the Runnable is not non-const reference, OTHT, the overload being removed checks nothing. Where, comments and tests asserts any *bound* argument should not be non-const reference, so the generic one has been checked excessively. This CL loosen the type check to match the requirement, and then the overloaded base::Bind is no longer needed. BUG= ==========
danakj@chromium.org changed reviewers: + danakj@chromium.org
LGTM! https://codereview.chromium.org/1512833002/diff/120001/base/bind.h File base/bind.h (right): https://codereview.chromium.org/1512833002/diff/120001/base/bind.h#newcode63 base/bind.h:63: // checks should below for bound references need to know what the actual nit: can you drop the out-of-place "should" in this comment while you're here https://codereview.chromium.org/1512833002/diff/120001/base/bind_unittest.cc File base/bind_unittest.cc (right): https://codereview.chromium.org/1512833002/diff/120001/base/bind_unittest.cc#... base/bind_unittest.cc:166: struct VoidPolymorphic { I'm not opposed, but why wrap the method in a struct here instead of just making the function variadic?
https://codereview.chromium.org/1512833002/diff/120001/base/bind.h File base/bind.h (right): https://codereview.chromium.org/1512833002/diff/120001/base/bind.h#newcode65 base/bind.h:65: typedef typename RunnableType::RunType BoundRunType; nit: maybe consider writing these typedefs with "using" so it's more consistent/better for reading, while you're here too.
https://codereview.chromium.org/1512833002/diff/120001/base/bind.h File base/bind.h (right): https://codereview.chromium.org/1512833002/diff/120001/base/bind.h#newcode63 base/bind.h:63: // checks should below for bound references need to know what the actual On 2015/12/16 20:00:41, danakj (behind on reviews) wrote: > nit: can you drop the out-of-place "should" in this comment while you're here Let me do it in a separate CL: https://codereview.chromium.org/1537553002 https://codereview.chromium.org/1512833002/diff/120001/base/bind.h#newcode65 base/bind.h:65: typedef typename RunnableType::RunType BoundRunType; On 2015/12/16 20:01:42, danakj (behind on reviews) wrote: > nit: maybe consider writing these typedefs with "using" so it's more > consistent/better for reading, while you're here too. ditto https://codereview.chromium.org/1512833002/diff/120001/base/bind_unittest.cc File base/bind_unittest.cc (right): https://codereview.chromium.org/1512833002/diff/120001/base/bind_unittest.cc#... base/bind_unittest.cc:166: struct VoidPolymorphic { On 2015/12/16 20:00:41, danakj (behind on reviews) wrote: > I'm not opposed, but why wrap the method in a struct here instead of just making > the function variadic? This is for gcc compilation error. Taking the function pointer of a function template with variadic template makes its instantiation ambiguous, since we can give template parameters partially for function template, unlike for class template. E.g. For |foo| below, foo<int> may mean "Ts contains only one parameter, which is int." or "Ts contains two parameters, the first one is int and the second should be deduced from the function arguments". I didn't look into the spec, but clang and cl take the first and gcc rejects to select that. template <typename... Ts> void foo(Ts...) {} foo<int>(); // Ts = <int> foo<int>(1, 2); // Ts = <int, int> &foo<int>; // ??
https://codereview.chromium.org/1512833002/diff/120001/base/bind_unittest.cc File base/bind_unittest.cc (right): https://codereview.chromium.org/1512833002/diff/120001/base/bind_unittest.cc#... base/bind_unittest.cc:166: struct VoidPolymorphic { On 2015/12/17 01:42:36, tzik wrote: > On 2015/12/16 20:00:41, danakj (behind on reviews) wrote: > > I'm not opposed, but why wrap the method in a struct here instead of just > making > > the function variadic? > > This is for gcc compilation error. > > Taking the function pointer of a function template with variadic template makes > its instantiation ambiguous, since we can give template parameters partially for > function template, unlike for class template. > > E.g. For |foo| below, foo<int> may mean "Ts contains only one parameter, which > is int." > or "Ts contains two parameters, the first one is int and the second should be > deduced from the function arguments". > I didn't look into the spec, but clang and cl take the first and gcc rejects to > select that. > > template <typename... Ts> > void foo(Ts...) {} > > foo<int>(); // Ts = <int> > foo<int>(1, 2); // Ts = <int, int> > &foo<int>; // ?? Ah, cool. Thanks!
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1512833002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1512833002/120001
Message was sent while issue was closed.
Description was changed from ========== Remove unbound base::Bind overload This CL removes a base::Bind overload without bound arguments. The difference to generic base::Bind is the type check for the bound Runnable: the generic one ensures any argument of the Runnable is not non-const reference, OTHT, the overload being removed checks nothing. Where, comments and tests asserts any *bound* argument should not be non-const reference, so the generic one has been checked excessively. This CL loosen the type check to match the requirement, and then the overloaded base::Bind is no longer needed. BUG= ========== to ========== Remove unbound base::Bind overload This CL removes a base::Bind overload without bound arguments. The difference to generic base::Bind is the type check for the bound Runnable: the generic one ensures any argument of the Runnable is not non-const reference, OTHT, the overload being removed checks nothing. Where, comments and tests asserts any *bound* argument should not be non-const reference, so the generic one has been checked excessively. This CL loosen the type check to match the requirement, and then the overloaded base::Bind is no longer needed. BUG= ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Remove unbound base::Bind overload This CL removes a base::Bind overload without bound arguments. The difference to generic base::Bind is the type check for the bound Runnable: the generic one ensures any argument of the Runnable is not non-const reference, OTHT, the overload being removed checks nothing. Where, comments and tests asserts any *bound* argument should not be non-const reference, so the generic one has been checked excessively. This CL loosen the type check to match the requirement, and then the overloaded base::Bind is no longer needed. BUG= ========== to ========== Remove unbound base::Bind overload This CL removes a base::Bind overload without bound arguments. The difference to generic base::Bind is the type check for the bound Runnable: the generic one ensures any argument of the Runnable is not non-const reference, OTHT, the overload being removed checks nothing. Where, comments and tests asserts any *bound* argument should not be non-const reference, so the generic one has been checked excessively. This CL loosen the type check to match the requirement, and then the overloaded base::Bind is no longer needed. BUG= Committed: https://crrev.com/7fe3a687ae2cd98c85e4556441b94e2268704e71 Cr-Commit-Position: refs/heads/master@{#365988} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/7fe3a687ae2cd98c85e4556441b94e2268704e71 Cr-Commit-Position: refs/heads/master@{#365988} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
