Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1507143003:
Clean up base::Callback stuff (Closed)
DescriptionClean up base::Callback stuff
* Typo fix
* Remove unused or duplicated forward decls
* Make Callback ctor explicit
* Simplify BindState param by expanding TypeList
BUG=
Committed: https://crrev.com/ce3ecf87d878414a8b475549c72ae799f1e1c5c9
Cr-Commit-Position: refs/heads/master@{#365184}
Patch Set 1 #Patch Set 2 : #
Total comments: 7
Patch Set 3 : #Patch Set 4 : revert Callback(BindStateType*) part #Patch Set 5 : fix #Patch Set 6 : #Patch Set 7 : fix nocompile test #
Total comments: 5
Patch Set 8 : #
Messages
Total messages: 45 (21 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/1507143003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1507143003/1
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/1507143003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1507143003/20001
Description was changed from ========== Clean up base::Callback stuff * Typo fix * Remove unused or duplicated forward decls * Make Callback ctor explicit * Simplify Callback ctor template param * Delegate Callback::Equals to CallbackBase BUG= ========== to ========== Clean up base::Callback stuff * Typo fix * Remove unused or duplicated forward decls * Make Callback ctor explicit * Simplify Callback ctor template param * Delegate Callback::Equals to CallbackBase * Simplify BindState param BUG= ==========
tzik@chromium.org changed reviewers: + dcheng@chromium.org
PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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/1507143003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1507143003/20001
dcheng@chromium.org changed reviewers: + danakj@chromium.org
+danakj@ as well. https://codereview.chromium.org/1507143003/diff/20001/base/bind_internal.h File base/bind_internal.h (right): https://codereview.chromium.org/1507143003/diff/20001/base/bind_internal.h#ne... base/bind_internal.h:365: template <typename Runnable, typename RunType, typename... BoundArg> This should be BoundArgs since there may be more than one. Also, the comment on line 363 needs to be updated. https://codereview.chromium.org/1507143003/diff/20001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/1507143003/diff/20001/base/callback.h#newcode367 base/callback.h:367: explicit Callback(BindStateType* bind_state) This would allow an external implementation of BindState to be instantiated rather than requiring internal::BindState: is there a use case for such a thing? https://codereview.chromium.org/1507143003/diff/20001/base/callback_internal.h File base/callback_internal.h (right): https://codereview.chromium.org/1507143003/diff/20001/base/callback_internal.... base/callback_internal.h:71: bool Equals(const CallbackBase& other) const; I'm curious: do you know why we didn't just do this to begin with?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
It might be nice to do each of these fixups in separate CLs so it's easier to see which parts of the CL are related. https://codereview.chromium.org/1507143003/diff/20001/base/callback_internal.h File base/callback_internal.h (right): https://codereview.chromium.org/1507143003/diff/20001/base/callback_internal.... base/callback_internal.h:71: bool Equals(const CallbackBase& other) const; On 2015/12/08 20:16:46, dcheng wrote: > I'm curious: do you know why we didn't just do this to begin with? Does this allow comparison of Callback<void(int)> and Callback<void(int, int)> to compile because they both upcast to CallbackBase?
Description was changed from ========== Clean up base::Callback stuff * Typo fix * Remove unused or duplicated forward decls * Make Callback ctor explicit * Simplify Callback ctor template param * Delegate Callback::Equals to CallbackBase * Simplify BindState param BUG= ========== to ========== Clean up base::Callback stuff * Typo fix * Remove unused or duplicated forward decls * Make Callback ctor explicit * Simplify Callback ctor template param * Simplify BindState param BUG= ==========
https://codereview.chromium.org/1507143003/diff/20001/base/bind_internal.h File base/bind_internal.h (right): https://codereview.chromium.org/1507143003/diff/20001/base/bind_internal.h#ne... base/bind_internal.h:365: template <typename Runnable, typename RunType, typename... BoundArg> On 2015/12/08 20:16:46, dcheng wrote: > This should be BoundArgs since there may be more than one. Also, the comment on > line 363 needs to be updated. Done. Also, removed the obsolete comments from line 42. https://codereview.chromium.org/1507143003/diff/20001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/1507143003/diff/20001/base/callback.h#newcode367 base/callback.h:367: explicit Callback(BindStateType* bind_state) On 2015/12/08 20:16:46, dcheng wrote: > This would allow an external implementation of BindState to be instantiated > rather than requiring internal::BindState: is there a use case for such a thing? No use case for now, except for callback_unittest.cc. But that will help us updating BindState incrementally. https://codereview.chromium.org/1507143003/diff/20001/base/callback_internal.h File base/callback_internal.h (right): https://codereview.chromium.org/1507143003/diff/20001/base/callback_internal.... base/callback_internal.h:71: bool Equals(const CallbackBase& other) const; On 2015/12/08 22:12:59, danakj (behind on reviews) wrote: > On 2015/12/08 20:16:46, dcheng wrote: > > I'm curious: do you know why we didn't just do this to begin with? > > Does this allow comparison of > > Callback<void(int)> and Callback<void(int, int)> to compile because they both > upcast to CallbackBase? Ah, yes. Let me revert this part, that is an unintended side effect of the clean up.
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/1507143003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1507143003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Clean up base::Callback stuff * Typo fix * Remove unused or duplicated forward decls * Make Callback ctor explicit * Simplify Callback ctor template param * Simplify BindState param BUG= ========== to ========== Clean up base::Callback stuff * Typo fix * Remove unused or duplicated forward decls * Make Callback ctor explicit * Simplify BindState param BUG= ==========
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/1507143003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1507143003/60001
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/1507143003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1507143003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
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/1507143003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1507143003/100001
lgtm one thing: update the description to remove the bullet point about simplifying the BindState param, since that will happen when it's needed. does this depend on https://codereview.chromium.org/1514573005?
https://codereview.chromium.org/1507143003/diff/120001/base/callback.h File base/callback.h (left): https://codereview.chromium.org/1507143003/diff/120001/base/callback.h#oldcod... base/callback.h:372: // Note that this constructor CANNOT be explicit, and that Bind() CANNOT Can you explain why this comment is no longer true? Because we're not using a TypeList for the bound args? Not sure how this relates..
Description was changed from ========== Clean up base::Callback stuff * Typo fix * Remove unused or duplicated forward decls * Make Callback ctor explicit * Simplify BindState param BUG= ========== to ========== Clean up base::Callback stuff * Typo fix * Remove unused or duplicated forward decls * Make Callback ctor explicit * Simplify BindState param by expanding TypeList BUG= ==========
On 2015/12/10 18:21:11, dcheng wrote: > lgtm > > one thing: update the description to remove the bullet point about simplifying > the BindState param, since that will happen when it's needed. > > does this depend on https://codereview.chromium.org/1514573005 The comment is for TypeList expandsion. I already removed Callback ctor templates at comment #23 (hidden description update log). This does not depends on that CL, but another CL does for nocompile test fix.
https://codereview.chromium.org/1507143003/diff/120001/base/callback.h File base/callback.h (left): https://codereview.chromium.org/1507143003/diff/120001/base/callback.h#oldcod... base/callback.h:372: // Note that this constructor CANNOT be explicit, and that Bind() CANNOT On 2015/12/10 19:13:16, danakj (behind on reviews) wrote: > Can you explain why this comment is no longer true? Because we're not using a > TypeList for the bound args? Not sure how this relates.. This is not related to TypeList or Variadic Template. The comment has been obsolete for 4 years since this CL: https://chromium.googlesource.com/chromium/src/+/e24f876c537646cab5a9e8492658...
LGTM https://codereview.chromium.org/1507143003/diff/120001/base/callback.h File base/callback.h (left): https://codereview.chromium.org/1507143003/diff/120001/base/callback.h#oldcod... base/callback.h:372: // Note that this constructor CANNOT be explicit, and that Bind() CANNOT On 2015/12/14 03:24:04, tzik wrote: > On 2015/12/10 19:13:16, danakj (behind on reviews) wrote: > > Can you explain why this comment is no longer true? Because we're not using a > > TypeList for the bound args? Not sure how this relates.. > > This is not related to TypeList or Variadic Template. > The comment has been obsolete for 4 years since this CL: > https://chromium.googlesource.com/chromium/src/+/e24f876c537646cab5a9e8492658... Thanks for context! https://codereview.chromium.org/1507143003/diff/120001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/1507143003/diff/120001/base/callback.h#newcod... base/callback.h:370: // See base/bind.h for details. I am not sure what details this comment is referring to anymore, it was talking about not being explicit before. I think you can remove it.
https://codereview.chromium.org/1507143003/diff/120001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/1507143003/diff/120001/base/callback.h#newcod... base/callback.h:370: // See base/bind.h for details. On 2015/12/14 21:06:08, danakj (behind on reviews) wrote: > I am not sure what details this comment is referring to anymore, it was talking > about not being explicit before. I think you can remove it. Done.
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1507143003/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1507143003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1507143003/140001
Message was sent while issue was closed.
Description was changed from ========== Clean up base::Callback stuff * Typo fix * Remove unused or duplicated forward decls * Make Callback ctor explicit * Simplify BindState param by expanding TypeList BUG= ========== to ========== Clean up base::Callback stuff * Typo fix * Remove unused or duplicated forward decls * Make Callback ctor explicit * Simplify BindState param by expanding TypeList BUG= ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Clean up base::Callback stuff * Typo fix * Remove unused or duplicated forward decls * Make Callback ctor explicit * Simplify BindState param by expanding TypeList BUG= ========== to ========== Clean up base::Callback stuff * Typo fix * Remove unused or duplicated forward decls * Make Callback ctor explicit * Simplify BindState param by expanding TypeList BUG= Committed: https://crrev.com/ce3ecf87d878414a8b475549c72ae799f1e1c5c9 Cr-Commit-Position: refs/heads/master@{#365184} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/ce3ecf87d878414a8b475549c72ae799f1e1c5c9 Cr-Commit-Position: refs/heads/master@{#365184} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
