Create ObjCCast<>() and ObjCCastStrict<>() methods

Create ObjCCast<>() and ObjCCastStrict<>() methods

These methods exist for casting a basic id to a more specific (NSObject derived) type.

BUG=86004
TEST=Initialise common NS objects and confirm ObjCCast*<>() casts to the expected result.


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

[KushalP, 2011-10-19 22:38:29]

This is very similar to the work complete on CFCast*<>().

There is one thing that I think needs to be done before it can be signed off: A
good example for ObjCCastStrict<>() to go in the docs in the header.

[Mark Mentovai, 2011-10-19 22:59:32]

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util.h#newc...
base/mac/foundation_util.h:230: // specific (NSObject derived) type. The
compatibility of the passed
Tiny nit: NSObject-derived, with a hyphen in there.

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util.h#newc...
base/mac/foundation_util.h:238: // NSNumber some_number =
base::mac::ObjCCast<NSNumber>(
NSNumber* for the type in the declaration, right?

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util.h#newc...
base/mac/foundation_util.h:241: // TODO: Write a good example for
ObjCCastStrict<>()
The strict variant is handy when you’re retrieving a value that you stuffed into
a collection yourself, and you know you’ve only stuffed values of a certain
type. For example, an NSArray of NSStrings. The non-strict variant is handy when
you’re retrieving values from data that you don’t fully control yourself. For
example, a plist read from disk may be beyond your exclusive control, so you’d
merely want to check that the values you pull from it are of the expected types,
but not crash if they’re not.

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util.h#newc...
base/mac/foundation_util.h:244: if (objc_val == NULL) {
Use nil for Objective-C. All of your uses of NULL throughout this new code and
its explanatory comment block above should be nil instead.

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util.h#newc...
base/mac/foundation_util.h:244: if (objc_val == NULL) {
Neat trick: you can remove this condition entirely!

Because it’s safe to send messages to nil receivers, and the return value (in
cases where the return type is “simple”—integral fundamental types are simple)
is zero. This means that you can call [nil isKindOfClass:[T class]] and it
returns NO. You’ll return NULL when you return from the function below.

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util.h#newc...
base/mac/foundation_util.h:262: #endif
#endif  // defined(__OBJC__)

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util_unitte...
File base/mac/foundation_util_unittest.mm (right):

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util_unitte...
base/mac/foundation_util_unittest.mm:164: id test_array = [NSArray
arrayWithObjects:
Let’s get a ScopedNSAutoreleasePool in here to make sure these objects are
deallocated when exiting the function.

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util_unitte...
base/mac/foundation_util_unittest.mm:173: dictionaryWithObject:[NSNumber
numberWithInt:42]
Formatting nit: I find it clearer to do
  id test_dict =
      [NSDictionary dictionaryWithObject:…

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util_unitte...
base/mac/foundation_util_unittest.mm:180: id test_str = @"bonjour";
As in the CF test:

This one is test_str_const. You can have test_str created from +[NSString
string] .

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util_unitte...
base/mac/foundation_util_unittest.mm:183: // Make sure the allocations of CF
types are good.
Adjust the comments because this test isn’t about CF.

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util_unitte...
base/mac/foundation_util_unittest.mm:214: // When given an incorrect ObjC cast,
provide NULL.
nil.

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util_unitte...
base/mac/foundation_util_unittest.mm:228: // Giving a NULL provides a NULL.
nil, and nil as the argument to each of these casts.

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util_unitte...
base/mac/foundation_util_unittest.mm:256: // ObjCCastStrict: Giving a NULL
provides a NULL.
And again here.

[KushalP, 2011-10-19 23:25:24]

All of the comments have been handled.

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util.h#newc...
base/mac/foundation_util.h:241: // TODO: Write a good example for
ObjCCastStrict<>()
All of this is incredibly useful. Should I also add it to the docs for others to
see?

On 2011/10/19 22:59:32, Mark Mentovai wrote:
> The strict variant is handy when you’re retrieving a value that you stuffed
into
> a collection yourself, and you know you’ve only stuffed values of a certain
> type. For example, an NSArray of NSStrings. The non-strict variant is handy
when
> you’re retrieving values from data that you don’t fully control yourself. For
> example, a plist read from disk may be beyond your exclusive control, so you’d
> merely want to check that the values you pull from it are of the expected
types,
> but not crash if they’re not.

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util.h#newc...
base/mac/foundation_util.h:244: if (objc_val == NULL) {
That is very cool!

A quick google for "nil isKindOfClass" didn't return anything useful, but is
this something that's talked about in the spec anywhere?

On 2011/10/19 22:59:32, Mark Mentovai wrote:
> Neat trick: you can remove this condition entirely!
> 
> Because it’s safe to send messages to nil receivers, and the return value (in
> cases where the return type is “simple”—integral fundamental types are simple)
> is zero. This means that you can call [nil isKindOfClass:[T class]] and it
> returns NO. You’ll return NULL when you return from the function below.

[Mark Mentovai, 2011-10-20 14:14:16]

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util.h#newc...
base/mac/foundation_util.h:244: if (objc_val == NULL) {
KushalP wrote:
> That is very cool!
> 
> A quick google for "nil isKindOfClass" didn't return anything useful, but is
> this something that's talked about in the spec anywhere?

It’s not a feature specific to -isKindOfClass:, it’s a general feature of the
language.

http://developer.apple.com/library/mac/#documentation/cocoa/conceptual/object...

http://codereview.chromium.org/8356024/diff/5/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8356024/diff/5/base/mac/foundation_util.h#newc...
base/mac/foundation_util.h:233: // requested return type, ObjCCast<>() returns
NULL and
Adjust the comment to reference nil instead of NULL as well.

http://codereview.chromium.org/8356024/diff/5/base/mac/foundation_util.h#newc...
base/mac/foundation_util.h:239: //     [dict objectForKey:@"object"]);
A good example for this would be:

NSString* version = base::mac::ObjCCast<NSString>([bundle
objectForInfoDictionaryKey:@"CFBundleShortVersionString"]);

(assuming bundle is an NSBundle*)

because the bundle’s Info.plist is not entirely within the program’s control,
and if some prankster puts a non-string into the plist for
CFBundleShortVersionString, the program might prefer to treat it the same as a
missing value rather than crashing.

http://codereview.chromium.org/8356024/diff/5/base/mac/foundation_util_unitte...
File base/mac/foundation_util_unittest.mm (right):

http://codereview.chromium.org/8356024/diff/5/base/mac/foundation_util_unitte...
base/mac/foundation_util_unittest.mm:167: id test_array = [NSArray
arrayWithObjects:
[NSArray array], [NSMutableArray array], [NSDictionary dictionary],
[NSMutableDictionary dictionary], [NSString string], [NSMutableString string]
etc. are fine. Remember how we discussed not confusing things with unnecessary
bits of set-up for the CF test?

http://codereview.chromium.org/8356024/diff/5/base/mac/foundation_util_unitte...
base/mac/foundation_util_unittest.mm:173: id test_data = [NSData data];
Why no test_data_mutable? You had a mutable version in the CF test.

[KushalP, 2011-10-20 14:56:33]

Comments handled and patch set added.

http://codereview.chromium.org/8356024/diff/5/base/mac/foundation_util_unitte...
File base/mac/foundation_util_unittest.mm (right):

http://codereview.chromium.org/8356024/diff/5/base/mac/foundation_util_unitte...
base/mac/foundation_util_unittest.mm:167: id test_array = [NSArray
arrayWithObjects:
I decided to choose slightly more interesting test data after you said "cafe"
was boring.

Made this cleaner now.

On 2011/10/20 14:14:16, Mark Mentovai wrote:
> [NSArray array], [NSMutableArray array], [NSDictionary dictionary],
> [NSMutableDictionary dictionary], [NSString string], [NSMutableString string]
> etc. are fine. Remember how we discussed not confusing things with unnecessary
> bits of set-up for the CF test?

http://codereview.chromium.org/8356024/diff/5/base/mac/foundation_util_unitte...
base/mac/foundation_util_unittest.mm:173: id test_data = [NSData data];
Added

On 2011/10/20 14:14:16, Mark Mentovai wrote:
> Why no test_data_mutable? You had a mutable version in the CF test.

[KushalP, 2011-10-20 14:59:37]

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util.h#newc...
base/mac/foundation_util.h:241: // TODO: Write a good example for
ObjCCastStrict<>()

You didn't state whether I should add what you said here. I think it's useful to
have the case for ObjCCastStrict written in prose like this. What do you think?

On 2011/10/19 23:25:24, KushalP wrote:
> All of this is incredibly useful. Should I also add it to the docs for others
to
> see?
> 
> On 2011/10/19 22:59:32, Mark Mentovai wrote:
> > The strict variant is handy when you’re retrieving a value that you stuffed
> into
> > a collection yourself, and you know you’ve only stuffed values of a certain
> > type. For example, an NSArray of NSStrings. The non-strict variant is handy
> when
> > you’re retrieving values from data that you don’t fully control yourself.
For
> > example, a plist read from disk may be beyond your exclusive control, so
you’d
> > merely want to check that the values you pull from it are of the expected
> types,
> > but not crash if they’re not.
>

[Mark Mentovai, 2011-10-21 14:30:54]

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util.h#newc...
base/mac/foundation_util.h:241: // TODO: Write a good example for
ObjCCastStrict<>()
KushalP wrote:
> You didn't state whether I should add what you said here. I think it's useful
to
> have the case for ObjCCastStrict written in prose like this. What do you
think?

What do you mean, like saying that the strict form is useful when you know
you’ve stuffed a container full of a certain type and just want to check the
objects on the way out? Sure.

[KushalP, 2011-10-21 23:50:28]

Pretty much! What I found particularly interesting was how you discussed the
case-based usage of ObjCCastStrict. By writing it out here like this it may make
it clearer for those who decide to use ObjCCast*<>() methods.

For me, this kind of (colloquial) docs would make it easier to reason about
using (or not) different methods in Chrom[ium|e].

On 2011/10/21 14:30:54, Mark Mentovai wrote:
> http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util.h
> File base/mac/foundation_util.h (right):
> 
>
http://codereview.chromium.org/8356024/diff/1/base/mac/foundation_util.h#newc...
> base/mac/foundation_util.h:241: // TODO: Write a good example for
> ObjCCastStrict<>()
> KushalP wrote:
> > You didn't state whether I should add what you said here. I think it's
useful
> to
> > have the case for ObjCCastStrict written in prose like this. What do you
> think?
> 
> What do you mean, like saying that the strict form is useful when you know
> you’ve stuffed a container full of a certain type and just want to check the
> objects on the way out? Sure.

[Mark Mentovai, 2011-10-22 16:59:13]

Right, I agree, I said “sure.” Let’s do it.

[KushalP, 2011-10-23 00:56:36]

On 2011/10/22 16:59:13, Mark Mentovai wrote:
> Right, I agree, I said “sure.” Let’s do it.

Updated with a docs path.

[KushalP, 2011-10-23 00:56:47]

On 2011/10/23 00:56:36, KushalP wrote:
> On 2011/10/22 16:59:13, Mark Mentovai wrote:
> > Right, I agree, I said “sure.” Let’s do it.
> 
> Updated with a docs path.

*patch

[Mark Mentovai, 2011-10-23 16:05:24]

http://codereview.chromium.org/8356024/diff/9003/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/8356024/diff/9003/base/mac/foundation_util.h#n...
base/mac/foundation_util.h:238: // stuffed into a collection, where you know
you’ve only stuffed
Your copy-and-paste job of my e-mail has resulted in non-ASCII characters
appearing here. This one’s a curly right single quote, and I see more on lines
242 and 244. We don’t check non-ASCII source in.

http://codereview.chromium.org/8356024/diff/9003/base/mac/foundation_util.h#n...
base/mac/foundation_util.h:241: // that aren't fully controlled yourself. For
example, a plist read
“data that aren’t fully controlled yourself” is nonsense—the “yourself” doesn’t
really refer to anything.

[KushalP, 2011-10-23 21:43:13]

Sorry. I've reworded it and removed the non-ASCII characters.

[Mark Mentovai, 2011-10-24 13:36:36]

LGTM  & CQ

[commit-bot: I haz the power, 2011-10-24 13:36:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kushi.p@gmail.com/8356024/12002

[commit-bot: I haz the power, 2011-10-24 14:39:06]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_rel, revision is 106910, job name
was 8356024-12002 (previous was lost) (previous was lost) (previous was lost)
(previous was lost).

[commit-bot: I haz the power, 2011-10-24 16:45:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kushi.p@gmail.com/8356024/12002

[commit-bot: I haz the power, 2011-10-24 18:04:17]

Change committed as 106946