Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9349016:
Update scoped_nsobject to be copyable. (Closed)
|
Created:
8 years, 10 months ago by qsr Modified:
8 years, 10 months ago Reviewers:
Mark Mentovai CC:
chromium-reviews, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUpdate scoped_nsobject to be copyable.
Having copyable scoped_nsobject allows to use those in C++ structure, in stl
datatypes as well as with the base::Callback API. This CL also introduces an
optional parameter to the constructor of scoped_nsobject to allow to state what
is the ownership policy.
BUG= none
TEST= memory/scoped_nsobject_mac_unittest.mm
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=121215
Patch Set 1 #Patch Set 2 : Correct documentation. #
Total comments: 32
Patch Set 3 : Follow mark review. #Patch Set 4 : Correcting following test results. #
Total comments: 10
Patch Set 5 : Follow review, #
Total comments: 2
Patch Set 6 : Spelling #
Total comments: 10
Patch Set 7 : Follow review. #
Messages
Total messages: 12 (0 generated)
https://chromiumcodereview.appspot.com/9349016/diff/1001/base/memory/scoped_n... File base/memory/scoped_nsobject.h (right): https://chromiumcodereview.appspot.com/9349016/diff/1001/base/memory/scoped_n... base/memory/scoped_nsobject.h:19: // constructor or in reset()), it takes over the caller's existing ownership Awkward phrasing. How ’bout By default, scoped_nsobject<> takes ownership of an object (in the constructor or in reset()) by taking over the caller's existing ownership claim. https://chromiumcodereview.appspot.com/9349016/diff/1001/base/memory/scoped_n... base/memory/scoped_nsobject.h:24: // scoped_nsobject<> will call |retain| on the object, and the initial -retain instead of |retain| https://chromiumcodereview.appspot.com/9349016/diff/1001/base/memory/scoped_n... base/memory/scoped_nsobject.h:25: // ownsershop is not changed. ownership, not ownsershop. https://chromiumcodereview.appspot.com/9349016/diff/1001/base/memory/scoped_n... base/memory/scoped_nsobject.h:37: ACQUIRE, Can we stuff this into a namespace? Having ACQUIRE and RETAIN available at the global scope seems like it’ll cause us to hit snags. https://chromiumcodereview.appspot.com/9349016/diff/1001/base/memory/scoped_n... base/memory/scoped_nsobject.h:45: : object_(policy == ACQUIRE ? object : [object retain]) { Here and on line 62: I can appreciate the terseness of how you’ve written it, but I wonder if it would read more clearly in a “say what you mean” sense if you wrote the assignment as object_(object) and then conditionally retained as a separate statement. The idea here is that |object| should always be assigned into |object_|, completely independent of what |policy| is. You and I both know that -retain just returns its receiver, so the code you’ve written is fine, but it sort of masks the actual intent. https://chromiumcodereview.appspot.com/9349016/diff/1001/base/memory/scoped_n... base/memory/scoped_nsobject.h:48: explicit scoped_nsprotocol(const scoped_nsprotocol<NST>& other) I don’t think this will work in STL containers with the copy constructor marked explicit. https://chromiumcodereview.appspot.com/9349016/diff/1001/base/memory/scoped_n... base/memory/scoped_nsobject.h:48: explicit scoped_nsprotocol(const scoped_nsprotocol<NST>& other) Here and on line 56: The other methods in this class that operate on another scoped_nsprotocol<> refer to the other one as |that| (lines 67 and 68). You should be consistent. https://chromiumcodereview.appspot.com/9349016/diff/1001/base/memory/scoped_n... base/memory/scoped_nsobject.h:56: scoped_nsprotocol& operator= (const scoped_nsprotocol<NST>& other) { No space between operator= and the (. https://chromiumcodereview.appspot.com/9349016/diff/1001/base/memory/scoped_n... base/memory/scoped_nsobject.h:120: explicit scoped_nsobject(const scoped_nsobject<NST>& other) The same comments as on line 48 applies here and on lines 124, 138, and 142. https://chromiumcodereview.appspot.com/9349016/diff/1001/base/memory/scoped_n... base/memory/scoped_nsobject.h:124: scoped_nsobject& operator= (const scoped_nsobject<NST>& other) { The same comment as on line 56 applies here and on line 142. https://chromiumcodereview.appspot.com/9349016/diff/1001/base/memory/scoped_n... base/memory/scoped_nsobject.h:128: }; Do we need the “free functions” (lines 97-111) for scoped_nsobject too? scoped_nsobject<id> as well? https://chromiumcodereview.appspot.com/9349016/diff/1001/base/memory/scoped_n... File base/memory/scoped_nsobject_mac_unittest.mm (right): https://chromiumcodereview.appspot.com/9349016/diff/1001/base/memory/scoped_n... base/memory/scoped_nsobject_mac_unittest.mm:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. This file doesn’t need _mac in the name. https://chromiumcodereview.appspot.com/9349016/diff/1001/base/memory/scoped_n... base/memory/scoped_nsobject_mac_unittest.mm:11: TEST(ScopedNsObjectTest, ScopedNSObject) { ScopedNSObjectTest, capital S. https://chromiumcodereview.appspot.com/9349016/diff/1001/base/memory/scoped_n... base/memory/scoped_nsobject_mac_unittest.mm:12: scoped_nsobject<NSObject> p1([NSObject alloc]); You should also explicitly test the two-argument constructor form to make sure that RETAIN works. https://chromiumcodereview.appspot.com/9349016/diff/1001/base/memory/scoped_n... base/memory/scoped_nsobject_mac_unittest.mm:12: scoped_nsobject<NSObject> p1([NSObject alloc]); Technically, you should also call -init. https://chromiumcodereview.appspot.com/9349016/diff/1001/base/memory/scoped_n... base/memory/scoped_nsobject_mac_unittest.mm:13: ASSERT_EQ(1, [p1 retainCount]); Before you do this, you should ASSERT_TRUE(p1) or ASSERT_TRUE(p1.get()).
http://codereview.chromium.org/9349016/diff/1001/base/memory/scoped_nsobject.h File base/memory/scoped_nsobject.h (right): http://codereview.chromium.org/9349016/diff/1001/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:19: // constructor or in reset()), it takes over the caller's existing ownership On 2012/02/07 17:13:17, Mark Mentovai wrote: > Awkward phrasing. How ’bout > > By default, scoped_nsobject<> takes ownership of an object (in the constructor > or in reset()) by taking over the caller's existing ownership claim. Done. http://codereview.chromium.org/9349016/diff/1001/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:24: // scoped_nsobject<> will call |retain| on the object, and the initial On 2012/02/07 17:13:17, Mark Mentovai wrote: > -retain instead of |retain| Done. http://codereview.chromium.org/9349016/diff/1001/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:25: // ownsershop is not changed. On 2012/02/07 17:13:17, Mark Mentovai wrote: > ownership, not ownsershop. Done. http://codereview.chromium.org/9349016/diff/1001/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:37: ACQUIRE, On 2012/02/07 17:13:17, Mark Mentovai wrote: > Can we stuff this into a namespace? Having ACQUIRE and RETAIN available at the > global scope seems like it’ll cause us to hit snags. Done. http://codereview.chromium.org/9349016/diff/1001/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:45: : object_(policy == ACQUIRE ? object : [object retain]) { On 2012/02/07 17:13:17, Mark Mentovai wrote: > Here and on line 62: > > I can appreciate the terseness of how you’ve written it, but I wonder if it > would read more clearly in a “say what you mean” sense if you wrote the > assignment as object_(object) and then conditionally retained as a separate > statement. The idea here is that |object| should always be assigned into > |object_|, completely independent of what |policy| is. You and I both know that > -retain just returns its receiver, so the code you’ve written is fine, but it > sort of masks the actual intent. Done. http://codereview.chromium.org/9349016/diff/1001/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:48: explicit scoped_nsprotocol(const scoped_nsprotocol<NST>& other) On 2012/02/07 17:13:17, Mark Mentovai wrote: > Here and on line 56: > > The other methods in this class that operate on another scoped_nsprotocol<> > refer to the other one as |that| (lines 67 and 68). You should be consistent. Done. http://codereview.chromium.org/9349016/diff/1001/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:48: explicit scoped_nsprotocol(const scoped_nsprotocol<NST>& other) On 2012/02/07 17:13:17, Mark Mentovai wrote: > I don’t think this will work in STL containers with the copy constructor marked > explicit. You are right. The funny thing is this compiles perfectly right, but the following: scoped_nsobject<T> p1(foo); scoped_nsobject<T> p2 = p1; will call the other constructor, with the default policy, which is exactly what we do not want. To have all working correctly, I have to remove the explicit from here, and add it to the first constructor. That allowed me to learn that explicit can have a use on multi parameters constructor. http://codereview.chromium.org/9349016/diff/1001/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:56: scoped_nsprotocol& operator= (const scoped_nsprotocol<NST>& other) { On 2012/02/07 17:13:17, Mark Mentovai wrote: > No space between operator= and the (. Done. http://codereview.chromium.org/9349016/diff/1001/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:120: explicit scoped_nsobject(const scoped_nsobject<NST>& other) On 2012/02/07 17:13:17, Mark Mentovai wrote: > The same comments as on line 48 applies here and on lines 124, 138, and 142. Done. http://codereview.chromium.org/9349016/diff/1001/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:124: scoped_nsobject& operator= (const scoped_nsobject<NST>& other) { On 2012/02/07 17:13:17, Mark Mentovai wrote: > The same comment as on line 56 applies here and on line 142. Done. http://codereview.chromium.org/9349016/diff/1001/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:128: }; I might be missing something, but why would we need it? As scoped_nsobject are scoped_nsprotocol by inheritance you can use all those free function on it. I'll add that to the test. http://codereview.chromium.org/9349016/diff/1001/base/memory/scoped_nsobject_... File base/memory/scoped_nsobject_mac_unittest.mm (right): http://codereview.chromium.org/9349016/diff/1001/base/memory/scoped_nsobject_... base/memory/scoped_nsobject_mac_unittest.mm:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/02/07 17:13:17, Mark Mentovai wrote: > This file doesn’t need _mac in the name. Done. http://codereview.chromium.org/9349016/diff/1001/base/memory/scoped_nsobject_... base/memory/scoped_nsobject_mac_unittest.mm:11: TEST(ScopedNsObjectTest, ScopedNSObject) { On 2012/02/07 17:13:17, Mark Mentovai wrote: > ScopedNSObjectTest, capital S. Done. http://codereview.chromium.org/9349016/diff/1001/base/memory/scoped_nsobject_... base/memory/scoped_nsobject_mac_unittest.mm:12: scoped_nsobject<NSObject> p1([NSObject alloc]); On 2012/02/07 17:13:17, Mark Mentovai wrote: > You should also explicitly test the two-argument constructor form to make sure > that RETAIN works. Done. http://codereview.chromium.org/9349016/diff/1001/base/memory/scoped_nsobject_... base/memory/scoped_nsobject_mac_unittest.mm:12: scoped_nsobject<NSObject> p1([NSObject alloc]); On 2012/02/07 17:13:17, Mark Mentovai wrote: > Technically, you should also call -init. Done. http://codereview.chromium.org/9349016/diff/1001/base/memory/scoped_nsobject_... base/memory/scoped_nsobject_mac_unittest.mm:13: ASSERT_EQ(1, [p1 retainCount]); On 2012/02/07 17:13:17, Mark Mentovai wrote: > Before you do this, you should ASSERT_TRUE(p1) or ASSERT_TRUE(p1.get()). Done.
http://codereview.chromium.org/9349016/diff/5001/base/memory/scoped_nsobject.h File base/memory/scoped_nsobject.h (right): http://codereview.chromium.org/9349016/diff/5001/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:36: namespace policy { You call it policy here but scoped_nsobject below on 41. policy is a bit generic. http://codereview.chromium.org/9349016/diff/5001/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:50: [object retain]; Use only a 2-space indent here. http://codereview.chromium.org/9349016/diff/5001/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:167: scoped_nsobject(NSAutoreleasePool* object = nil, Mark this explicit to match the others? http://codereview.chromium.org/9349016/diff/5001/base/memory/scoped_nsobject_... File base/memory/scoped_nsobject_unittest.mm (right): http://codereview.chromium.org/9349016/diff/5001/base/memory/scoped_nsobject_... base/memory/scoped_nsobject_unittest.mm:36: TEST(ScopedNSObjectTest, ScopedNSObjectInContainer) { Perhaps this test could be a little bit more involved, possibly involving an object that outlives the vector, to test that adding it to the vector increments the retain count, and destroying the vector decrements it. http://codereview.chromium.org/9349016/diff/5001/base/memory/scoped_nsobject_... base/memory/scoped_nsobject_unittest.mm:49: scoped_nsobject<id> p2([[NSObject alloc] init]); At this point, test that o1 != p2 and o1 == p2 produce the expected results as well.
http://codereview.chromium.org/9349016/diff/5001/base/memory/scoped_nsobject.h File base/memory/scoped_nsobject.h (right): http://codereview.chromium.org/9349016/diff/5001/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:36: namespace policy { On 2012/02/08 15:24:00, Mark Mentovai wrote: > You call it policy here but scoped_nsobject below on 41. > > policy is a bit generic. Done. http://codereview.chromium.org/9349016/diff/5001/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:50: [object retain]; On 2012/02/08 15:24:00, Mark Mentovai wrote: > Use only a 2-space indent here. Done. http://codereview.chromium.org/9349016/diff/5001/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:167: scoped_nsobject(NSAutoreleasePool* object = nil, On 2012/02/08 15:24:00, Mark Mentovai wrote: > Mark this explicit to match the others? Done. http://codereview.chromium.org/9349016/diff/5001/base/memory/scoped_nsobject_... File base/memory/scoped_nsobject_unittest.mm (right): http://codereview.chromium.org/9349016/diff/5001/base/memory/scoped_nsobject_... base/memory/scoped_nsobject_unittest.mm:36: TEST(ScopedNSObjectTest, ScopedNSObjectInContainer) { On 2012/02/08 15:24:00, Mark Mentovai wrote: > Perhaps this test could be a little bit more involved, possibly involving an > object that outlives the vector, to test that adding it to the vector increments > the retain count, and destroying the vector decrements it. Done. http://codereview.chromium.org/9349016/diff/5001/base/memory/scoped_nsobject_... base/memory/scoped_nsobject_unittest.mm:49: scoped_nsobject<id> p2([[NSObject alloc] init]); On 2012/02/08 15:24:00, Mark Mentovai wrote: > At this point, test that o1 != p2 and o1 == p2 produce the expected results as > well. Done.
drive by https://chromiumcodereview.appspot.com/9349016/diff/8001/base/memory/scoped_n... File base/memory/scoped_nsobject.h (right): https://chromiumcodereview.appspot.com/9349016/diff/8001/base/memory/scoped_n... base/memory/scoped_nsobject.h:25: // ownsership is not changed. spelling: ownership
http://codereview.chromium.org/9349016/diff/8001/base/memory/scoped_nsobject.h File base/memory/scoped_nsobject.h (right): http://codereview.chromium.org/9349016/diff/8001/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:25: // ownsership is not changed. On 2012/02/08 15:56:16, Avi wrote: > spelling: ownership Done.
This is now looking pretty strong. http://codereview.chromium.org/9349016/diff/6004/base/memory/scoped_nsobject.h File base/memory/scoped_nsobject.h (right): http://codereview.chromium.org/9349016/diff/6004/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:27: // scoped_nsprotocol<> has the same behavior than scoped_nsobject, but can be than → as http://codereview.chromium.org/9349016/diff/6004/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:38: ACQUIRE, “Acquire” can be misunderstood to mean that scoped_nsobject<> will acquire a new reference. I think this should be renamed ASSUME. http://codereview.chromium.org/9349016/diff/6004/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:58: virtual ~scoped_nsprotocol() { Why is this marked virtual? It shouldn’t have to be. http://codereview.chromium.org/9349016/diff/6004/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:97: // wrapper for [object_ release]. To force a scoped_nsprotocol<> call This is missing a word, “to.” It should read: To force a scoped_nsprotocol<> to call http://codereview.chromium.org/9349016/diff/6004/base/memory/scoped_nsobject_... File base/memory/scoped_nsobject_unittest.mm (right): http://codereview.chromium.org/9349016/diff/6004/base/memory/scoped_nsobject_... base/memory/scoped_nsobject_unittest.mm:34: } Let’s test the in-class operator== and operator!= here too.
http://codereview.chromium.org/9349016/diff/6004/base/memory/scoped_nsobject.h File base/memory/scoped_nsobject.h (right): http://codereview.chromium.org/9349016/diff/6004/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:27: // scoped_nsprotocol<> has the same behavior than scoped_nsobject, but can be On 2012/02/08 17:03:05, Mark Mentovai wrote: > than → as Done. http://codereview.chromium.org/9349016/diff/6004/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:38: ACQUIRE, On 2012/02/08 17:03:05, Mark Mentovai wrote: > “Acquire” can be misunderstood to mean that scoped_nsobject<> will acquire a new > reference. I think this should be renamed ASSUME. Done. http://codereview.chromium.org/9349016/diff/6004/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:58: virtual ~scoped_nsprotocol() { Kind of a reflex as soon as there is inheritance. Removed, in the hope that nobody will ever implement a destructor on the sub classes. http://codereview.chromium.org/9349016/diff/6004/base/memory/scoped_nsobject.... base/memory/scoped_nsobject.h:97: // wrapper for [object_ release]. To force a scoped_nsprotocol<> call On 2012/02/08 17:03:05, Mark Mentovai wrote: > This is missing a word, “to.” It should read: > > To force a scoped_nsprotocol<> to call Done. http://codereview.chromium.org/9349016/diff/6004/base/memory/scoped_nsobject_... File base/memory/scoped_nsobject_unittest.mm (right): http://codereview.chromium.org/9349016/diff/6004/base/memory/scoped_nsobject_... base/memory/scoped_nsobject_unittest.mm:34: } On 2012/02/08 17:03:05, Mark Mentovai wrote: > Let’s test the in-class operator== and operator!= here too. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qsr@chromium.org/9349016/4006
Change committed as 121215 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
