Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2034633002:
Decouple Invoker from BindState (Closed)
DescriptionDecouple Invoker from BindState
Move most of type-level logic out of BindState, and decouple Invoker
from BindState. So that we can use BindState with another Invoker impl.
- UnboundRunType calculation is moved to a separate helper template as
MakeUnboundRuntype.
- Indices generation to extract tuple is moved from BindState to
Invoker.
- WeakPtr handling is moved from BindState to Invoker.
This is a preparation CL as well to implement a OneShot variant of
Callback. That will share the same Callback and BindState impl with
different Invoker::Run impl and different Callback tag as Copyable
Callback and MoveOnly Callback have.
BUG=554299
Committed: https://crrev.com/caf1d84bb83aaf5369eb508027a685e2bf9859b4
Cr-Commit-Position: refs/heads/master@{#402446}
Patch Set 1 #Patch Set 2 : +comment. Remove InvokeOneShot for now. #Patch Set 3 : fix #Patch Set 4 : attempt to avoid MSVC failure #Patch Set 5 : fix #Patch Set 6 : attempt2 #Patch Set 7 : attempt3 #Patch Set 8 : comment fix #Patch Set 9 : inline for size bloat #Patch Set 10 : more inline #
Total comments: 7
Patch Set 11 : comment fix. s/static inline/inline/. #Patch Set 12 : rebase #
Total comments: 4
Patch Set 13 : rebase #Patch Set 14 : update #
Total comments: 4
Patch Set 15 : revert IsWeakMethod<> change #Patch Set 16 : remove base::internal::MakeUnboundRunType #
Messages
Total messages: 37 (20 generated)
Description was changed from ========== BindState mod TODO: Description BUG= ========== to ========== Simplify BindState 0 1 2 3 4 5 6 7 123456789012345678901234567890123456789012345678901234567890123456789012 Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to separate helper template as MakeUnboundRuntype. - Convert Invoker to a function from a struct. - Indices generation to extract tuple is moved from BindState to Invoke(). - WeakPtr handling is moved from BindState to InvokeImpl(). BUG= ==========
Description was changed from ========== Simplify BindState 0 1 2 3 4 5 6 7 123456789012345678901234567890123456789012345678901234567890123456789012 Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to separate helper template as MakeUnboundRuntype. - Convert Invoker to a function from a struct. - Indices generation to extract tuple is moved from BindState to Invoke(). - WeakPtr handling is moved from BindState to InvokeImpl(). BUG= ========== to ========== Simplify BindState Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to a separate helper template as MakeUnboundRuntype. - Indices generation to extract tuple is moved from BindState to Invoker. - WeakPtr handling is moved from BindState to Invoker. BUG= ==========
Description was changed from ========== Simplify BindState Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to a separate helper template as MakeUnboundRuntype. - Indices generation to extract tuple is moved from BindState to Invoker. - WeakPtr handling is moved from BindState to Invoker. BUG= ========== to ========== Simplify BindState Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to a separate helper template as MakeUnboundRuntype. - Indices generation to extract tuple is moved from BindState to Invoker. - WeakPtr handling is moved from BindState to Invoker. BUG=554299 ==========
tzik@chromium.org changed reviewers: + dcheng@chromium.org
Description was changed from ========== Simplify BindState Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to a separate helper template as MakeUnboundRuntype. - Indices generation to extract tuple is moved from BindState to Invoker. - WeakPtr handling is moved from BindState to Invoker. BUG=554299 ========== to ========== Simplify BindState Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to a separate helper template as MakeUnboundRuntype. - Indices generation to extract tuple is moved from BindState to Invoker. - WeakPtr handling is moved from BindState to Invoker. TODO: - Check binary size diff BUG=554299 ==========
Description was changed from ========== Simplify BindState Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to a separate helper template as MakeUnboundRuntype. - Indices generation to extract tuple is moved from BindState to Invoker. - WeakPtr handling is moved from BindState to Invoker. TODO: - Check binary size diff BUG=554299 ========== to ========== Simplify BindState Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to a separate helper template as MakeUnboundRuntype. - Indices generation to extract tuple is moved from BindState to Invoker. - WeakPtr handling is moved from BindState to Invoker. TODO: - Check binary size diff ? -> 164738336 (unstripped) ? -> 117083728 (--strip-unneeded) BUG=554299 ==========
Description was changed from ========== Simplify BindState Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to a separate helper template as MakeUnboundRuntype. - Indices generation to extract tuple is moved from BindState to Invoker. - WeakPtr handling is moved from BindState to Invoker. TODO: - Check binary size diff ? -> 164738336 (unstripped) ? -> 117083728 (--strip-unneeded) BUG=554299 ========== to ========== Simplify BindState Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to a separate helper template as MakeUnboundRuntype. - Indices generation to extract tuple is moved from BindState to Invoker. - WeakPtr handling is moved from BindState to Invoker. TODO: - Check binary size diff 164359048 -> 164738336 (unstripped) 117014096 -> 117083728 (--strip-unneeded) BUG=554299 ==========
Description was changed from ========== Simplify BindState Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to a separate helper template as MakeUnboundRuntype. - Indices generation to extract tuple is moved from BindState to Invoker. - WeakPtr handling is moved from BindState to Invoker. TODO: - Check binary size diff 164359048 -> 164738336 (unstripped) 117014096 -> 117083728 (--strip-unneeded) BUG=554299 ========== to ========== Simplify BindState Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to a separate helper template as MakeUnboundRuntype. - Indices generation to extract tuple is moved from BindState to Invoker. - WeakPtr handling is moved from BindState to Invoker. TODO: - Check binary size diff 164359048 -> 164738336 (unstripped), +379288 117014096 -> 117083728 (--strip-unneeded), +69632B BUG=554299 ==========
Description was changed from ========== Simplify BindState Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to a separate helper template as MakeUnboundRuntype. - Indices generation to extract tuple is moved from BindState to Invoker. - WeakPtr handling is moved from BindState to Invoker. TODO: - Check binary size diff 164359048 -> 164738336 (unstripped), +379288 117014096 -> 117083728 (--strip-unneeded), +69632B BUG=554299 ========== to ========== Simplify BindState Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to a separate helper template as MakeUnboundRuntype. - Indices generation to extract tuple is moved from BindState to Invoker. - WeakPtr handling is moved from BindState to Invoker. TODO: - Check the stripped binary size diff 117014096 -> 117005904, -8192B BUG=554299 ==========
Description was changed from ========== Simplify BindState Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to a separate helper template as MakeUnboundRuntype. - Indices generation to extract tuple is moved from BindState to Invoker. - WeakPtr handling is moved from BindState to Invoker. TODO: - Check the stripped binary size diff 117014096 -> 117005904, -8192B BUG=554299 ========== to ========== Simplify BindState Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to a separate helper template as MakeUnboundRuntype. - Indices generation to extract tuple is moved from BindState to Invoker. - WeakPtr handling is moved from BindState to Invoker. BUG=554299 ==========
tzik@chromium.org changed reviewers: + thakis@chromium.org
PTAL
generally looks good https://codereview.chromium.org/2034633002/diff/180001/base/bind.h File base/bind.h (right): https://codereview.chromium.org/2034633002/diff/180001/base/bind.h#newcode53 base/bind.h:53: static inline base::Callback<MakeUnboundRunType<Functor, Args...>> (looks like this bit was part of the other cl as well) https://codereview.chromium.org/2034633002/diff/180001/base/bind_helpers.h File base/bind_helpers.h (right): https://codereview.chromium.org/2034633002/diff/180001/base/bind_helpers.h#ne... base/bind_helpers.h:568: // A type-level function that extracts return type of a function. grammar nit: "that extracts the return type of a function" ("a of b" almost always requires a "the" in front of "a", except if "b" is so broad to not add information to "a" -- for example "people from all over the world" doesn't have a "the" in front because of "all over the the world" doesn't add information to "people") https://codereview.chromium.org/2034633002/diff/180001/base/bind_internal.h File base/bind_internal.h (right): https://codereview.chromium.org/2034633002/diff/180001/base/bind_internal.h#n... base/bind_internal.h:302: static inline ReturnType MakeItSo(Runnable&& runnable, RunArgs&&... args) { "static inline" here makes more sense to me, as it's in a class :-) I still wonder why we need to add inline to a function like this. It's so short, I would've expected it to be inlined by default. https://codereview.chromium.org/2034633002/diff/180001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/2034633002/diff/180001/base/callback.h#newcod... base/callback.h:357: // Revisit this after we update it to newer version. we could revisit this now
https://codereview.chromium.org/2034633002/diff/180001/base/bind_internal.h File base/bind_internal.h (right): https://codereview.chromium.org/2034633002/diff/180001/base/bind_internal.h#n... base/bind_internal.h:302: static inline ReturnType MakeItSo(Runnable&& runnable, RunArgs&&... args) { On 2016/06/02 17:27:44, Nico wrote: > "static inline" here makes more sense to me, as it's in a class :-) I still > wonder why we need to add inline to a function like this. It's so short, I > would've expected it to be inlined by default. I thought so too. I don't understand why these inline makes >70kB diff on the stripped chrome binary... Let me look into it tomorrow.
It looks like thakis@ already reviewed this pretty thoroughly. My only comment is if we can update the CL description to include the 'why' of this change: why do we want another Invoker implementation?
Description was changed from ========== Simplify BindState Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to a separate helper template as MakeUnboundRuntype. - Indices generation to extract tuple is moved from BindState to Invoker. - WeakPtr handling is moved from BindState to Invoker. BUG=554299 ========== to ========== Decouple Invoker from BindState Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to a separate helper template as MakeUnboundRuntype. - Indices generation to extract tuple is moved from BindState to Invoker. - WeakPtr handling is moved from BindState to Invoker. This is a preparation CL as well to implement a OneShot variant of Callback. That will share same Callback and BindState impl with different Invoker::Run impl and different Callback tag as Copyable Callback and MoveOnly Callback have. BUG=554299 ==========
Description was changed from ========== Decouple Invoker from BindState Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to a separate helper template as MakeUnboundRuntype. - Indices generation to extract tuple is moved from BindState to Invoker. - WeakPtr handling is moved from BindState to Invoker. This is a preparation CL as well to implement a OneShot variant of Callback. That will share same Callback and BindState impl with different Invoker::Run impl and different Callback tag as Copyable Callback and MoveOnly Callback have. BUG=554299 ========== to ========== Decouple Invoker from BindState Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to a separate helper template as MakeUnboundRuntype. - Indices generation to extract tuple is moved from BindState to Invoker. - WeakPtr handling is moved from BindState to Invoker. This is a preparation CL as well to implement a OneShot variant of Callback. That will share the same Callback and BindState impl with different Invoker::Run impl and different Callback tag as Copyable Callback and MoveOnly Callback have. BUG=554299 ==========
https://codereview.chromium.org/2034633002/diff/180001/base/bind_helpers.h File base/bind_helpers.h (right): https://codereview.chromium.org/2034633002/diff/180001/base/bind_helpers.h#ne... base/bind_helpers.h:568: // A type-level function that extracts return type of a function. On 2016/06/02 17:27:44, Nico wrote: > grammar nit: "that extracts the return type of a function" ("a of b" almost > always requires a "the" in front of "a", except if "b" is so broad to not add > information to "a" -- for example "people from all over the world" doesn't have > a "the" in front because of "all over the the world" doesn't add information to > "people") Thanks! Done. https://codereview.chromium.org/2034633002/diff/180001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/2034633002/diff/180001/base/callback.h#newcod... base/callback.h:357: // Revisit this after we update it to newer version. On 2016/06/02 17:27:44, Nico wrote: > we could revisit this now Let me do it in a separate CL.
tzik@chromium.org changed reviewers: + yutak@chromium.org
Adding yutak@, who refactored WTF::bind() recently.
LGTM with nits, though I'm not a pro of base::Bind. https://codereview.chromium.org/2034633002/diff/220001/base/bind_internal.h File base/bind_internal.h (right): https://codereview.chromium.org/2034633002/diff/220001/base/bind_internal.h#n... base/bind_internal.h:356: constexpr size_t num_bound_args = nit: Add "static" since constexpr doesn't imply static. Ditto for below. https://codereview.chromium.org/2034633002/diff/220001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/2034633002/diff/220001/base/callback.h#newcod... base/callback.h:355: super nit: This newline is probably not needed. You might want to add one before "public:".
https://codereview.chromium.org/2034633002/diff/220001/base/bind_internal.h File base/bind_internal.h (right): https://codereview.chromium.org/2034633002/diff/220001/base/bind_internal.h#n... base/bind_internal.h:356: constexpr size_t num_bound_args = On 2016/06/23 07:22:45, Yuta Kitamura wrote: > nit: Add "static" since constexpr doesn't imply static. Ditto for below. Hmm, I think that does not make any diff in this case. Done anyway. https://codereview.chromium.org/2034633002/diff/220001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/2034633002/diff/220001/base/callback.h#newcod... base/callback.h:355: On 2016/06/23 07:22:45, Yuta Kitamura wrote: > super nit: This newline is probably not needed. You might want to add > one before "public:". Done.
dcheng, thakis: Any chance to take another look?
https://codereview.chromium.org/2034633002/diff/260001/base/bind.h File base/bind.h (right): https://codereview.chromium.org/2034633002/diff/260001/base/bind.h#newcode50 base/bind.h:50: using MakeUnboundRunType = internal::MakeUnboundRunType<Functor, Args...>; Nit: It seems a little unusual to have base::MakeUnboundRunTYpe and base::internal::MakeUnboundRunType. I wonder if we can just qualify references with internal:: as needed? https://codereview.chromium.org/2034633002/diff/260001/base/bind_helpers.h File base/bind_helpers.h (right): https://codereview.chromium.org/2034633002/diff/260001/base/bind_helpers.h#ne... base/bind_helpers.h:465: template <bool IsMethod, typename ArgsTuple> Just for my own understanding, why did this template argument change?
https://codereview.chromium.org/2034633002/diff/260001/base/bind.h File base/bind.h (right): https://codereview.chromium.org/2034633002/diff/260001/base/bind.h#newcode50 base/bind.h:50: using MakeUnboundRunType = internal::MakeUnboundRunType<Functor, Args...>; On 2016/06/27 07:57:22, dcheng wrote: > Nit: It seems a little unusual to have base::MakeUnboundRunTYpe and > base::internal::MakeUnboundRunType. I wonder if we can just qualify references > with internal:: as needed? We have several place that uses MakeUnboundRunType outside of //base: wtf/Functional.h and gmock_mutant.h. How about unifying them to base::MakeUnboundRunType rather than base::internal:: one? https://codereview.chromium.org/2034633002/diff/260001/base/bind_helpers.h File base/bind_helpers.h (right): https://codereview.chromium.org/2034633002/diff/260001/base/bind_helpers.h#ne... base/bind_helpers.h:465: template <bool IsMethod, typename ArgsTuple> On 2016/06/27 07:57:22, dcheng wrote: > Just for my own understanding, why did this template argument change? That is for utility for the caller, Invoker::RunImpl, which has the tuple of bound arguments, but doesn't have unpacked tuple elements. Maybe, it's less confusing to do it in the caller side rather than modifying IsWeakMethod?
LGTM
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yutak@chromium.org Link to the patchset: https://codereview.chromium.org/2034633002/#ps300001 (title: "remove base::internal::MakeUnboundRunType")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by tzik@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Decouple Invoker from BindState Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to a separate helper template as MakeUnboundRuntype. - Indices generation to extract tuple is moved from BindState to Invoker. - WeakPtr handling is moved from BindState to Invoker. This is a preparation CL as well to implement a OneShot variant of Callback. That will share the same Callback and BindState impl with different Invoker::Run impl and different Callback tag as Copyable Callback and MoveOnly Callback have. BUG=554299 ========== to ========== Decouple Invoker from BindState Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to a separate helper template as MakeUnboundRuntype. - Indices generation to extract tuple is moved from BindState to Invoker. - WeakPtr handling is moved from BindState to Invoker. This is a preparation CL as well to implement a OneShot variant of Callback. That will share the same Callback and BindState impl with different Invoker::Run impl and different Callback tag as Copyable Callback and MoveOnly Callback have. BUG=554299 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Decouple Invoker from BindState Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to a separate helper template as MakeUnboundRuntype. - Indices generation to extract tuple is moved from BindState to Invoker. - WeakPtr handling is moved from BindState to Invoker. This is a preparation CL as well to implement a OneShot variant of Callback. That will share the same Callback and BindState impl with different Invoker::Run impl and different Callback tag as Copyable Callback and MoveOnly Callback have. BUG=554299 ========== to ========== Decouple Invoker from BindState Move most of type-level logic out of BindState, and decouple Invoker from BindState. So that we can use BindState with another Invoker impl. - UnboundRunType calculation is moved to a separate helper template as MakeUnboundRuntype. - Indices generation to extract tuple is moved from BindState to Invoker. - WeakPtr handling is moved from BindState to Invoker. This is a preparation CL as well to implement a OneShot variant of Callback. That will share the same Callback and BindState impl with different Invoker::Run impl and different Callback tag as Copyable Callback and MoveOnly Callback have. BUG=554299 Committed: https://crrev.com/caf1d84bb83aaf5369eb508027a685e2bf9859b4 Cr-Commit-Position: refs/heads/master@{#402446} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/caf1d84bb83aaf5369eb508027a685e2bf9859b4 Cr-Commit-Position: refs/heads/master@{#402446} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
