scoped_nsobject

Add scoped_nsobject to help handle ownership of NSObject subclass objects by
C++ code.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=12283

[Mark Mentovai, 2009-03-23 14:56:09]

Came up in codereview 50074.  Thought I'd make myself useful.

[Avi (use Gerrit), 2009-03-23 14:59:48]

/me likes...

lgtm

[TVL, 2009-03-23 15:03:34]

lgtm

(we were just talking about needing some of these this morning)

[Avi (use Gerrit), 2009-03-23 15:25:16]

Do these go into the gyp file?

[tommi (sloooow) - chröme, 2009-03-23 15:37:03]

drive-by...

http://codereview.chromium.org/52014/diff/1/3
File base/scoped_nsobject.h (right):

http://codereview.chromium.org/52014/diff/1/3#newcode8
Line 8: #import <Foundation/Foundation.h>
um... #import?

http://codereview.chromium.org/52014/diff/1/3#newcode28
Line 28: [object_ release];
what's going on here?

[Avi (use Gerrit), 2009-03-23 15:38:58]

http://codereview.chromium.org/52014/diff/1/3
File base/scoped_nsobject.h (right):

http://codereview.chromium.org/52014/diff/1/3#newcode28
Line 28: [object_ release];
On 2009/03/23 15:37:03, tommi wrote:
> what's going on here?

This is Objective C++ code, not plain C++. Please familiarize yourself with that
language and its coding style before commenting.

[Avi (use Gerrit), 2009-03-23 15:40:38]

Mark—

In using this in a class of mine, I'm getting the error:

../base/scoped_nsobject.h:72: error: ISO C++ forbids declaration of
'DISALLOW_COPY_AND_ASSIGN' with no type

[Scott Hess - ex-Googler, 2009-03-23 16:54:17]

LVGTM, with the blow caveat.

http://codereview.chromium.org/52014/diff/1/3
File base/scoped_nsobject.h (right):

http://codereview.chromium.org/52014/diff/1/3#newcode31
Line 31: void reset(NST* object = nil) {
I think you always need to -release, due to the rule about passing ownership in.
 Consider:

scoped_nsobject x([anObject retain]);
x.reset([anObject retain]);

Or am I missing something about that?

Then again, the rule about passing ownership in means that you don't need to get
funky with ordering or -release and assignment, because if object and object_
are the same object, there should be two or more references.

[Mark Mentovai, 2009-03-23 17:03:48]

http://codereview.chromium.org/52014/diff/1/3
File base/scoped_nsobject.h (right):

http://codereview.chromium.org/52014/diff/1/3#newcode31
Line 31: void reset(NST* object = nil) {
shess wrote:
> I think you always need to -release, due to the rule about passing ownership
in.

Interesting point.  The ownership rule was always sort of implicit, now I'm glad
I documented it because I think you're right.  The caller should be careful to
only own objects it passes in to here.  Basically:

bad:
scoped_nsobject<NST> x([[NST alloc] init]);
x.reset(x.get());

good:
scoped_nsobject<NST> x([[NST alloc] init]);
x.reset([x.get() retain]);

[tommi (sloooow) - chröme, 2009-03-23 17:54:59]

rubber stamp lgtm as I don't want to block.

The drive-by was because this is a header file in base and there are no OS
guards in the file that protect it from being included from cross platform code.
 Amazing how that managed to turn ugly all of a sudden.

Platform specific files in base have a postfix. I think _mac would be
appropriate here.

The "#import" statement is replacing #includes.  #import is recognized by the vc
compiler on windows, but does a completely different thing.

Aside from using the objective c++ specific [] way of calling methods, the file
contains standard C++.  Maybe the class could have a template traits class to
factor out the mac specific bits?  Just an idea.

http://codereview.chromium.org/52014/diff/1/3
File base/scoped_nsobject.h (right):

http://codereview.chromium.org/52014/diff/1/3#newcode28
Line 28: [object_ release];
On 2009/03/23 15:38:58, Avi wrote:
> On 2009/03/23 15:37:03, tommi wrote:
> > what's going on here?
> 
> This is Objective C++ code, not plain C++. Please familiarize yourself with
that
> language and its coding style before commenting.

seriously?

[Avi (use Gerrit), 2009-03-23 18:18:50]

On 2009/03/23 17:54:59, tommi wrote:
> The drive-by was because this is a header file in base and there are no OS
> guards in the file that protect it from being included from cross platform
code.

There are lots of header files in base/ that are platform-specific that have no
guards.

> Aside from using the objective c++ specific [] way of calling methods, the
file
> contains standard C++.

Objective C++ is a superset of C++, and so almost any ObjC++ file will have some
standard C++ in it.

> Maybe the class could have a template traits class to
> factor out the mac specific bits?  Just an idea.

We were looking for a small .h to do ownership of an ObjC object. This header is
useful only for ObjC code, so you can safely ignore it if you don't need it.

Some segregation of platform files might be a good idea; bring it up on the list
for discussion.

[Mark Mentovai, 2009-03-23 18:53:06]

tommi:
>Platform specific files in base have a postfix. I think _mac would be
>appropriate here.

avi:
>Some segregation of platform files might be a good idea; bring it up on the
>list for discussion.

I agree and I support this.  We've generally been moving in the right direction
here and it would reduce the number of files we'd need to list explicitly in
per-platform conditionals in our .gyp files.  It's less important for headers
and we have a handful in base that don't follow this style, though, so I was
just matching what we already do.

If you do bring this up on the list I'd be behind you.  If you just want to
start cleaning these up and sending out changelists, I'd happily lg them.