Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1993433002:
Adds equality operators for comparing scoped_refptr with nullptr. (Closed)
|
Created:
4 years, 7 months ago by Rob Percival Modified:
4 years, 7 months ago Reviewers:
danakj CC:
chromium-reviews, gavinp+memory_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds equality operators for comparing scoped_refptr with nullptr.
Committed: https://crrev.com/3d1a0004b6d793eb91c438b6058dfbbcf44f4dd7
Cr-Commit-Position: refs/heads/master@{#394620}
Patch Set 1 #Patch Set 2 : Adds operators for handling nullptr on lhs #
Total comments: 6
Patch Set 3 : Addresses review comments #
Dependent Patchsets: Messages
Total messages: 20 (6 generated)
The CQ bit was checked by robpercival@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/1993433002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993433002/1
The CQ bit was checked by robpercival@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/1993433002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993433002/20001
robpercival@chromium.org changed reviewers: + danakj@chromium.org
PTAL
Oh interesting, I'd have expected this to work since you can implicitly convert to scoped_refptr, but I guess it does't know what U to use when it just has a nullptr (also doesn't work for NULL). Can you add some unit tests that show this works now? https://codereview.chromium.org/1993433002/diff/20001/base/memory/ref_counted.h File base/memory/ref_counted.h (right): https://codereview.chromium.org/1993433002/diff/20001/base/memory/ref_counted... base/memory/ref_counted.h:448: return lhs.get() == null; can you implement this in terms of already existing code return !static_cast<bool>(lhs) should do https://codereview.chromium.org/1993433002/diff/20001/base/memory/ref_counted... base/memory/ref_counted.h:453: return null == rhs.get(); ditto
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/05/17 23:14:08, danakj wrote: > Oh interesting, I'd have expected this to work since you can implicitly convert > to scoped_refptr, but I guess it does't know what U to use when it just has a > nullptr (also doesn't work for NULL). > > Can you add some unit tests that show this works now? > > https://codereview.chromium.org/1993433002/diff/20001/base/memory/ref_counted.h > File base/memory/ref_counted.h (right): > > https://codereview.chromium.org/1993433002/diff/20001/base/memory/ref_counted... > base/memory/ref_counted.h:448: return lhs.get() == null; > can you implement this in terms of already existing code > > return !static_cast<bool>(lhs) > > should do > > https://codereview.chromium.org/1993433002/diff/20001/base/memory/ref_counted... > base/memory/ref_counted.h:453: return null == rhs.get(); > ditto You'd be right to expect that (mostly) - nullptr implicitly converts to U* on most systems and the existing equality operators just work. However, on the Mac build slaves, that doesn't happen for some reason and we get compilation failures. Adding these explicit equality operators fixes that. What would you like unit tested, specifically?
On Tue, May 17, 2016 at 6:03 PM, <robpercival@chromium.org> wrote: > On 2016/05/17 23:14:08, danakj wrote: > > Oh interesting, I'd have expected this to work since you can implicitly > convert > > to scoped_refptr, but I guess it does't know what U to use when it just > has a > > nullptr (also doesn't work for NULL). > > > > Can you add some unit tests that show this works now? > > > > > > https://codereview.chromium.org/1993433002/diff/20001/base/memory/ref_counted.h > > File base/memory/ref_counted.h (right): > > > > > > https://codereview.chromium.org/1993433002/diff/20001/base/memory/ref_counted... > > base/memory/ref_counted.h:448: return lhs.get() == null; > > can you implement this in terms of already existing code > > > > return !static_cast<bool>(lhs) > > > > should do > > > > > > https://codereview.chromium.org/1993433002/diff/20001/base/memory/ref_counted... > > base/memory/ref_counted.h:453: return null == rhs.get(); > > ditto > > You'd be right to expect that (mostly) - nullptr implicitly converts to U* > on > most systems and the existing equality operators just work. However, on > the Mac > build slaves, that doesn't happen for some reason and we get compilation > failures. Adding these explicit equality operators fixes that. > > What would you like unit tested, specifically? > Just a test that calls each of these operators and EXPECTs their outcomes, where the scoped_refptr is set and where it's null. > > https://codereview.chromium.org/1993433002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/05/18 01:06:38, danakj wrote: > On Tue, May 17, 2016 at 6:03 PM, <mailto:robpercival@chromium.org> wrote: > > > On 2016/05/17 23:14:08, danakj wrote: > > > Oh interesting, I'd have expected this to work since you can implicitly > > convert > > > to scoped_refptr, but I guess it does't know what U to use when it just > > has a > > > nullptr (also doesn't work for NULL). > > > > > > Can you add some unit tests that show this works now? > > > > > > > > > > > https://codereview.chromium.org/1993433002/diff/20001/base/memory/ref_counted.h > > > File base/memory/ref_counted.h (right): > > > > > > > > > > > https://codereview.chromium.org/1993433002/diff/20001/base/memory/ref_counted... > > > base/memory/ref_counted.h:448: return lhs.get() == null; > > > can you implement this in terms of already existing code > > > > > > return !static_cast<bool>(lhs) > > > > > > should do > > > > > > > > > > > https://codereview.chromium.org/1993433002/diff/20001/base/memory/ref_counted... > > > base/memory/ref_counted.h:453: return null == rhs.get(); > > > ditto > > > > You'd be right to expect that (mostly) - nullptr implicitly converts to U* > > on > > most systems and the existing equality operators just work. However, on > > the Mac > > build slaves, that doesn't happen for some reason and we get compilation > > failures. Adding these explicit equality operators fixes that. > > > > What would you like unit tested, specifically? > > > > Just a test that calls each of these operators and EXPECTs their outcomes, > where the scoped_refptr is set and where it's null. > > > > > > https://codereview.chromium.org/1993433002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I've already done that - see my addition to base/memory/ref_counted_unittest.cc?
> I've already done that - see my addition to base/memory/ref_counted_unittest.cc? Oh, I missed that sorry. That looks good. I had one comment about the header file, and here's one for the test. LG otherwise. https://codereview.chromium.org/1993433002/diff/20001/base/memory/ref_counted... File base/memory/ref_counted_unittest.cc (right): https://codereview.chromium.org/1993433002/diff/20001/base/memory/ref_counted... base/memory/ref_counted_unittest.cc:209: scoped_refptr<SelfAssign> null_ptr; nit: ptr_to_nullptr might be easier to read here
https://codereview.chromium.org/1993433002/diff/20001/base/memory/ref_counted.h File base/memory/ref_counted.h (right): https://codereview.chromium.org/1993433002/diff/20001/base/memory/ref_counted... base/memory/ref_counted.h:448: return lhs.get() == null; On 2016/05/17 23:14:08, danakj wrote: > can you implement this in terms of already existing code > > return !static_cast<bool>(lhs) > > should do Done. https://codereview.chromium.org/1993433002/diff/20001/base/memory/ref_counted... base/memory/ref_counted.h:453: return null == rhs.get(); On 2016/05/17 23:14:08, danakj wrote: > ditto Done. https://codereview.chromium.org/1993433002/diff/20001/base/memory/ref_counted... File base/memory/ref_counted_unittest.cc (right): https://codereview.chromium.org/1993433002/diff/20001/base/memory/ref_counted... base/memory/ref_counted_unittest.cc:209: scoped_refptr<SelfAssign> null_ptr; On 2016/05/18 01:12:34, danakj wrote: > nit: ptr_to_nullptr might be easier to read here Done.
LGTM
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/1993433002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993433002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Adds equality operators for comparing scoped_refptr with nullptr. ========== to ========== Adds equality operators for comparing scoped_refptr with nullptr. Committed: https://crrev.com/3d1a0004b6d793eb91c438b6058dfbbcf44f4dd7 Cr-Commit-Position: refs/heads/master@{#394620} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3d1a0004b6d793eb91c438b6058dfbbcf44f4dd7 Cr-Commit-Position: refs/heads/master@{#394620} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
