|
|
DescriptionMember initialization for scoped_refptr<T>::ptr_.
Also augment OpaqueRefCounted tests to cover move constructors
and RefCountedThreadSafe.
BUG=653916
Committed: https://crrev.com/37af9ea6080ce52e001189255512bd5bfa1c6c45
Cr-Commit-Position: refs/heads/master@{#428746}
Patch Set 1 #Patch Set 2 : +move_assigned #Patch Set 3 : {} instead of default? #
Total comments: 4
Patch Set 4 : remove extraneous TestOpaqueRefCounted calls #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 36 (25 generated)
Description was changed from ========== Member initialization for scoped_refptr<T>::ptr_. Also add RefCountedThreadSafe to OpaqueRefCounted tests BUG=653916 ========== to ========== Member initialization for scoped_refptr<T>::ptr_. Also augment OpaqueRefCounted tests to cover move constructors and RefCountedThreadSafe. BUG=653916 ==========
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: 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 gab@chromium.org
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
gab@chromium.org changed reviewers: + danakj@chromium.org
Dana PTAL, for some reason which I don't understand: {} compiles but not = default; ?! (see PS1) Improved coverage of OpaqueRefCounted tests which initially led me to believe full-type definition isn't required after all... but it appears to depend on the declaration of extern template class scoped_refptr<base::OpaqueRefCounted>; which I haven't found a single matching instance of in the codebase: $ git grep -n "template class scoped_refptr" base/memory/ref_counted.h:391: // extern template class scoped_refptr<Opaque>; base/test/opaque_ref_counted.cc:58:template class scoped_refptr<base::OpaqueRefCounted>; base/test/opaque_ref_counted.cc:59:template class scoped_refptr<base::OpaqueRefCountedThreadSafe>; base/test/opaque_ref_counted.h:26:extern template class scoped_refptr<base::OpaqueRefCounted>; base/test/opaque_ref_counted.h:27:extern template class scoped_refptr<base::OpaqueRefCountedThreadSafe>;
On 2016/10/27 18:34:34, gab wrote: > Dana PTAL, for some reason which I don't understand: {} compiles but not = > default; ?! (see PS1) > > Improved coverage of OpaqueRefCounted tests which initially led me to believe > full-type definition isn't required after all... but it appears to depend on the > declaration of > > extern template class scoped_refptr<base::OpaqueRefCounted>; > > which I haven't found a single matching instance of in the codebase: > > $ git grep -n "template class scoped_refptr" > base/memory/ref_counted.h:391: // extern template class > scoped_refptr<Opaque>; > base/test/opaque_ref_counted.cc:58:template class > scoped_refptr<base::OpaqueRefCounted>; > base/test/opaque_ref_counted.cc:59:template class > scoped_refptr<base::OpaqueRefCountedThreadSafe>; > base/test/opaque_ref_counted.h:26:extern template class > scoped_refptr<base::OpaqueRefCounted>; > base/test/opaque_ref_counted.h:27:extern template class > scoped_refptr<base::OpaqueRefCountedThreadSafe>; FYI, I plan to figure exactly how to do the scoped_refptr fwd-declaration in https://codereview.chromium.org/2443103003/ and then inform chromium-dev.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/10/27 18:37:51, gab wrote: > On 2016/10/27 18:34:34, gab wrote: > > Dana PTAL, for some reason which I don't understand: {} compiles but not = > > default; ?! (see PS1) = default isn't user-defined so it ends up tickling different parts of compilers. Found a bug today where xcode's compiler can't handle a constexpr = default constructor but can handle constexpr {} so.. yah. > > Improved coverage of OpaqueRefCounted tests which initially led me to believe > > full-type definition isn't required after all... but it appears to depend on > the > > declaration of > > > > extern template class scoped_refptr<base::OpaqueRefCounted>; > > > > which I haven't found a single matching instance of in the codebase: > > > > $ git grep -n "template class scoped_refptr" > > base/memory/ref_counted.h:391: // extern template class > > scoped_refptr<Opaque>; > > base/test/opaque_ref_counted.cc:58:template class > > scoped_refptr<base::OpaqueRefCounted>; > > base/test/opaque_ref_counted.cc:59:template class > > scoped_refptr<base::OpaqueRefCountedThreadSafe>; > > base/test/opaque_ref_counted.h:26:extern template class > > scoped_refptr<base::OpaqueRefCounted>; > > base/test/opaque_ref_counted.h:27:extern template class > > scoped_refptr<base::OpaqueRefCountedThreadSafe>; > > FYI, I plan to figure exactly how to do the scoped_refptr fwd-declaration in > https://codereview.chromium.org/2443103003/ and then inform chromium-dev.
https://codereview.chromium.org/2446403006/diff/40001/base/memory/ref_counted... File base/memory/ref_counted_unittest.cc (right): https://codereview.chromium.org/2446403006/diff/40001/base/memory/ref_counted... base/memory/ref_counted_unittest.cc:173: scoped_refptr<base::OpaqueRefCounted> moved(std::move(initial)); Isn't this just testing different ways to make a scoped_refptr? Once you have one, all called to TestOpaqueRefCounted are the same at that point?
https://codereview.chromium.org/2446403006/diff/40001/base/memory/ref_counted... File base/memory/ref_counted_unittest.cc (right): https://codereview.chromium.org/2446403006/diff/40001/base/memory/ref_counted... base/memory/ref_counted_unittest.cc:173: scoped_refptr<base::OpaqueRefCounted> moved(std::move(initial)); On 2016/10/27 22:10:54, danakj wrote: > Isn't this just testing different ways to make a scoped_refptr? Once you have > one, all called to TestOpaqueRefCounted are the same at that point? Right I guess the multiple calls to TestOpaqueRefCounted() aren't verifying much (i.e. the test is mostly about making sure it compiles not about logic testing so the multiple calls aren't necessarily helpful -- other than to verify the move happened correctly but other tests are already testing move so this is perhaps overkill, WDYT?)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2446403006/diff/40001/base/memory/ref_counted... File base/memory/ref_counted_unittest.cc (right): https://codereview.chromium.org/2446403006/diff/40001/base/memory/ref_counted... base/memory/ref_counted_unittest.cc:173: scoped_refptr<base::OpaqueRefCounted> moved(std::move(initial)); On 2016/10/28 14:02:26, gab wrote: > On 2016/10/27 22:10:54, danakj wrote: > > Isn't this just testing different ways to make a scoped_refptr? Once you have > > one, all called to TestOpaqueRefCounted are the same at that point? > > Right I guess the multiple calls to TestOpaqueRefCounted() aren't verifying much > (i.e. the test is mostly about making sure it compiles not about logic testing > so the multiple calls aren't necessarily helpful -- other than to verify the > move happened correctly but other tests are already testing move so this is > perhaps overkill, WDYT?) Yes if u wanna test them do it as a separate test if its not redundant with other tests, but no need to call TestOpaqueRefCounted after each one :)
LGTM otherwise
Thanks, done https://codereview.chromium.org/2446403006/diff/40001/base/memory/ref_counted... File base/memory/ref_counted_unittest.cc (right): https://codereview.chromium.org/2446403006/diff/40001/base/memory/ref_counted... base/memory/ref_counted_unittest.cc:173: scoped_refptr<base::OpaqueRefCounted> moved(std::move(initial)); On 2016/10/29 00:49:09, danakj wrote: > On 2016/10/28 14:02:26, gab wrote: > > On 2016/10/27 22:10:54, danakj wrote: > > > Isn't this just testing different ways to make a scoped_refptr? Once you > have > > > one, all called to TestOpaqueRefCounted are the same at that point? > > > > Right I guess the multiple calls to TestOpaqueRefCounted() aren't verifying > much > > (i.e. the test is mostly about making sure it compiles not about logic testing > > so the multiple calls aren't necessarily helpful -- other than to verify the > > move happened correctly but other tests are already testing move so this is > > perhaps overkill, WDYT?) > > Yes if u wanna test them do it as a separate test if its not redundant with > other tests, but no need to call TestOpaqueRefCounted after each one :) Removed extraneous calls.
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2446403006/#ps60001 (title: "remove extraneous TestOpaqueRefCounted calls")
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 ========== Member initialization for scoped_refptr<T>::ptr_. Also augment OpaqueRefCounted tests to cover move constructors and RefCountedThreadSafe. BUG=653916 ========== to ========== Member initialization for scoped_refptr<T>::ptr_. Also augment OpaqueRefCounted tests to cover move constructors and RefCountedThreadSafe. BUG=653916 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Member initialization for scoped_refptr<T>::ptr_. Also augment OpaqueRefCounted tests to cover move constructors and RefCountedThreadSafe. BUG=653916 ========== to ========== Member initialization for scoped_refptr<T>::ptr_. Also augment OpaqueRefCounted tests to cover move constructors and RefCountedThreadSafe. BUG=653916 Committed: https://crrev.com/37af9ea6080ce52e001189255512bd5bfa1c6c45 Cr-Commit-Position: refs/heads/master@{#428746} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/37af9ea6080ce52e001189255512bd5bfa1c6c45 Cr-Commit-Position: refs/heads/master@{#428746} |