Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1658813003:
Replace base::Callback forward decl with callback_forward.h (Closed)
|
Created:
4 years, 10 months ago by tzik Modified:
4 years, 10 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, skanuj+watch_chromium.org, melevin+watch_chromium.org, vmpstr+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, jfweitz+watch_chromium.org, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReplace base::Callback forward decl with callback_forward.h
base::Callback will have extra template parameters with default values. But
manual forward declaration causes a compile error on the parameter addition.
This CL unifies the forward decls for its preparation.
BUG=554299
Committed: https://crrev.com/a87bd2fc736d4d528ba3d85ecc8862d74a65c81a
Cr-Commit-Position: refs/heads/master@{#374733}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 3
Patch Set 7 : rebase #Patch Set 8 : revert #
Messages
Total messages: 42 (19 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/1658813003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1658813003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
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/1658813003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1658813003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_gn_chromeos_rel 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/1658813003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1658813003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_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/1658813003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1658813003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/1658813003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1658813003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_TIMED_OUT, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== Move base::Callback to base::internal and rename it to CallbackImpl BUG= ========== to ========== Move base::Callback to an internal namespace * Move and rename base::Callback to base::internal::CallbackImpl * Add base::Callback as an alias to CallbackImpl. So that we can extend Callback incrementally without exposing its intermediate implementation. BUG= ==========
Description was changed from ========== Move base::Callback to an internal namespace * Move and rename base::Callback to base::internal::CallbackImpl * Add base::Callback as an alias to CallbackImpl. So that we can extend Callback incrementally without exposing its intermediate implementation. BUG= ========== to ========== Move base::Callback to an internal namespace * Move and rename base::Callback to base::internal::CallbackImpl * Add base::Callback as an alias to CallbackImpl. So that we can extend Callback incrementally without exposing its intermediate implementation. BUG=554299 ==========
tzik@chromium.org changed reviewers: + ananta@chromium.org, qinmin@chromium.org, thakis@chromium.org
PTAL. I'm extending base::Callback for movable types (WIP CL: http://crrev.com/1689543004). But before that, I want to move it into an internal namespace to avoid exposing unready variants. thakis: //base and //chrome qinmin: //media/base/android ananta: //win8
media/base/android lgtm
https://codereview.chromium.org/1658813003/diff/100001/base/callback_forward.h File base/callback_forward.h (right): https://codereview.chromium.org/1658813003/diff/100001/base/callback_forward.... base/callback_forward.h:17: using Callback = internal::CallbackImpl<Signature>; Won't ADL mean that this doesn't give you any protection? (I think typedefs don't block ADL, do they?)
The non-base bits lgtm. base lgtm if my ADL comment is incorrect or if there's some other good reason for doing this that I misunderstood.
https://codereview.chromium.org/1658813003/diff/100001/base/callback_forward.h File base/callback_forward.h (right): https://codereview.chromium.org/1658813003/diff/100001/base/callback_forward.... base/callback_forward.h:17: using Callback = internal::CallbackImpl<Signature>; On 2016/02/10 20:10:41, Nico wrote: > Won't ADL mean that this doesn't give you any protection? (I think typedefs > don't block ADL, do they?) Do you mean, we'll happen to be able to call base::internal::Foo below as Foo(Callback()) without using "internal"? namespace base {namespace internal { void Foo(CallbackImpl); }} Yes, we can. I wasn't aware that, but it looks confusing to me.
https://codereview.chromium.org/1658813003/diff/100001/base/callback_forward.h File base/callback_forward.h (right): https://codereview.chromium.org/1658813003/diff/100001/base/callback_forward.... base/callback_forward.h:17: using Callback = internal::CallbackImpl<Signature>; On 2016/02/10 20:32:35, tzik wrote: > On 2016/02/10 20:10:41, Nico wrote: > > Won't ADL mean that this doesn't give you any protection? (I think typedefs > > don't block ADL, do they?) > > Do you mean, we'll happen to be able to call base::internal::Foo below as > Foo(Callback()) without using "internal"? > > namespace base {namespace internal { > void Foo(CallbackImpl); > }} > > Yes, we can. I wasn't aware that, but it looks confusing to me. Right, I mean since base::Callback is just a typedef for base::internal::CallbackImpl, ADL means (I think) that the internal namespace doesn't give us any protection – anything that uses base::Callback will have access to base::internal too. (Didn't test that though, so not 100% sure.) If that's true, I don't understand what the advantage of the internal namespace is.
On 2016/02/10 20:36:19, Nico wrote: > https://codereview.chromium.org/1658813003/diff/100001/base/callback_forward.h > File base/callback_forward.h (right): > > https://codereview.chromium.org/1658813003/diff/100001/base/callback_forward.... > base/callback_forward.h:17: using Callback = internal::CallbackImpl<Signature>; > On 2016/02/10 20:32:35, tzik wrote: > > On 2016/02/10 20:10:41, Nico wrote: > > > Won't ADL mean that this doesn't give you any protection? (I think typedefs > > > don't block ADL, do they?) > > > > Do you mean, we'll happen to be able to call base::internal::Foo below as > > Foo(Callback()) without using "internal"? > > > > namespace base {namespace internal { > > void Foo(CallbackImpl); > > }} > > > > Yes, we can. I wasn't aware that, but it looks confusing to me. > > Right, I mean since base::Callback is just a typedef for > base::internal::CallbackImpl, ADL means (I think) that the internal namespace > doesn't give us any protection – anything that uses base::Callback will have > access to base::internal too. (Didn't test that though, so not 100% sure.) If > that's true, I don't understand what the advantage of the internal namespace is. Yes, that's true. Let me take down the //base part of the CL per this discussion. The purpose of the namespace migration was to protect CallbackImpl from using unexpected combination of (to-be-added) flags, and to remove redundant "internal::". But, we can make it without the namespace migration anyway. On this way, the necessary parts are //chrome/browser/chromeos change only.
Description was changed from ========== Move base::Callback to an internal namespace * Move and rename base::Callback to base::internal::CallbackImpl * Add base::Callback as an alias to CallbackImpl. So that we can extend Callback incrementally without exposing its intermediate implementation. BUG=554299 ========== to ========== Replace base::Callback forward decl with callback_forward.h base::Callback will have extra template parameters with default values. But manual forward declaration causes a compile error on the parameter addition. This CL unifies the forward decls for its preparation. BUG=554299 ==========
Updated! I reverted the rename and namespace migration, and updated the CL description.
lgtm (nit: please wrap cl description to 80 cols)
Description was changed from ========== Replace base::Callback forward decl with callback_forward.h base::Callback will have extra template parameters with default values. But manual forward declaration causes a compile error on the parameter addition. This CL unifies the forward decls for its preparation. BUG=554299 ========== to ========== Replace base::Callback forward decl with callback_forward.h base::Callback will have extra template parameters with default values. But manual forward declaration causes a compile error on the parameter addition. This CL unifies the forward decls for its preparation. BUG=554299 ==========
On 2016/02/10 21:06:05, Nico wrote: > lgtm > > (nit: please wrap cl description to 80 cols) Done!
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org Link to the patchset: https://codereview.chromium.org/1658813003/#ps140001 (title: "revert")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1658813003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1658813003/140001
Message was sent while issue was closed.
Description was changed from ========== Replace base::Callback forward decl with callback_forward.h base::Callback will have extra template parameters with default values. But manual forward declaration causes a compile error on the parameter addition. This CL unifies the forward decls for its preparation. BUG=554299 ========== to ========== Replace base::Callback forward decl with callback_forward.h base::Callback will have extra template parameters with default values. But manual forward declaration causes a compile error on the parameter addition. This CL unifies the forward decls for its preparation. BUG=554299 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Replace base::Callback forward decl with callback_forward.h base::Callback will have extra template parameters with default values. But manual forward declaration causes a compile error on the parameter addition. This CL unifies the forward decls for its preparation. BUG=554299 ========== to ========== Replace base::Callback forward decl with callback_forward.h base::Callback will have extra template parameters with default values. But manual forward declaration causes a compile error on the parameter addition. This CL unifies the forward decls for its preparation. BUG=554299 Committed: https://crrev.com/a87bd2fc736d4d528ba3d85ecc8862d74a65c81a Cr-Commit-Position: refs/heads/master@{#374733} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/a87bd2fc736d4d528ba3d85ecc8862d74a65c81a Cr-Commit-Position: refs/heads/master@{#374733} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
