Create a nicer interface for base::mac::GetValueFromDictionary

base/mac/foundation_util.h

Index: base/mac/foundation_util.h
diff --git a/base/mac/foundation_util.h b/base/mac/foundation_util.h
index f7821de08cdb09bca58aaca59da82bd689407e29..52ee5d842266c2336d5eb097ebb8fff65d37fa90 100644
--- a/base/mac/foundation_util.h
+++ b/base/mac/foundation_util.h
@@ -13,6 +13,7 @@
 
 #include "base/base_export.h"
 #include "base/logging.h"
+#include "base/mac/scoped_cftyperef.h"
 
 #if defined(__OBJC__)
 #import <Foundation/Foundation.h>
@@ -96,6 +97,26 @@ BASE_EXPORT FilePath GetUserLibraryPath();
 //   returns - path to the application bundle, or empty on error
 BASE_EXPORT FilePath GetAppBundlePath(const FilePath& exec_name);
 
+#define TYPE_NAME_FOR_CF_TYPE_DECL(TypeCF) \
+std::string TypeNameForCFType(TypeCF##Ref cfo);

[Mark Mentovai, 2011/11/14 19:29:14]

The argument is unused and thus doesn’t even need to be named. Here and in the
definition in the .mm file.

+
+TYPE_NAME_FOR_CF_TYPE_DECL(CFArray);
+TYPE_NAME_FOR_CF_TYPE_DECL(CFBag);
+TYPE_NAME_FOR_CF_TYPE_DECL(CFBoolean);
+TYPE_NAME_FOR_CF_TYPE_DECL(CFData);
+TYPE_NAME_FOR_CF_TYPE_DECL(CFDate);
+TYPE_NAME_FOR_CF_TYPE_DECL(CFDictionary);
+TYPE_NAME_FOR_CF_TYPE_DECL(CFNull);
+TYPE_NAME_FOR_CF_TYPE_DECL(CFNumber);
+TYPE_NAME_FOR_CF_TYPE_DECL(CFSet);
+TYPE_NAME_FOR_CF_TYPE_DECL(CFString);
+

[Mark Mentovai, 2011/11/14 19:29:14]

You should #undef TYPE_NAME_FOR_CF_TYPE_DECL at this point.

See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Prepro...
.

You can add #undefs for the other macros used the same way in this .h file and
the .mm file.

+// Helper function for GetValueFromDictionary to create the error message
+// that appears when a type mismatch is encountered.
+std::string GetValueFromDictionaryErrorMessage(CFStringRef key,
+                                               std::string expected_type_ref,

[Mark Mentovai, 2011/11/14 18:59:29]

This can accept a const&, can’t it?

[Mark Mentovai, 2011/11/14 19:29:14]

This can be const std::string&, right?

There’s nothing “reffy” about it, the variable can just be |expected_type|.

+                                               CFTypeRef value);
+
 // Utility function to pull out a value from a dictionary, check its type, and
 // return it.  Returns NULL if the key is not present or of the wrong type.
 BASE_EXPORT CFTypeRef GetValueFromDictionary(CFDictionaryRef dict,

[Mark Mentovai, 2011/11/14 19:29:14]

The goal is to entirely replace this function, correct?

If so, mark it as deprecated in this change, and your next change should be a
simple no-contest change to convert all existing callers to the new API.

[KushalP, 2011/11/14 20:37:13]

How do I mark as deprecated in this case? Is it just adding __attribute__
((deprecated)) to the end of the method?

[Mark Mentovai, 2011/11/14 20:49:22]

KushalP wrote:
Just put a comment in.

And it won’t stay there for long, because your next change will be to convert
all existing callers over to the new API.

[KushalP, 2011/11/14 21:04:12]

Not in one change I hope! That'll be git rebase hell until it's committed.

[Mark Mentovai, 2011/11/14 21:09:01]

KushalP wrote:
I don’t know or care about that, but I mean “change” in the sense that other
developers will see, that is, svn commits.

@@ -218,10 +239,10 @@ namespace mac {
 //     base::mac::GetValueFromDictionary(some_dict,
 //                                       CFSTR("a_key"),
 //                                       CFStringGetTypeID()));
-BASE_EXPORT template<class T>
+BASE_EXPORT template<typename T>
 T CFCast(const CFTypeRef& cf_val);
 
-BASE_EXPORT template<class T>
+BASE_EXPORT template<typename T>
 T CFCastStrict(const CFTypeRef& cf_val);
 
 #if defined(__OBJC__)
@@ -248,7 +269,7 @@ T CFCastStrict(const CFTypeRef& cf_val);
 //
 // NSString* str = base::mac::ObjCCastStrict<NSString>(
 //     [ns_arr_of_ns_strs objectAtIndex:0]);
-BASE_EXPORT template<class T>
+BASE_EXPORT template<typename T>
 T* ObjCCast(id objc_val) {
   if ([objc_val isKindOfClass:[T class]]) {
     return reinterpret_cast<T*>(objc_val);
@@ -256,7 +277,7 @@ T* ObjCCast(id objc_val) {
   return nil;
 }
 
-BASE_EXPORT template<class T>
+BASE_EXPORT template<typename T>
 T* ObjCCastStrict(id objc_val) {
   T* rv = ObjCCast<T>(objc_val);
   DCHECK(objc_val == nil || rv);
@@ -265,6 +286,23 @@ T* ObjCCastStrict(id objc_val) {
 
 #endif  // defined(__OBJC__)
 
+// Utility function to pull out a value from a dictionary, check its type, and

[Mark Mentovai, 2011/11/14 19:29:14]

Why is this all the way down here, and not up with the GetValueWithDictionary
that it will replace?

[KushalP, 2011/11/14 20:37:13]

because it uses CFCast which isn't defined above this point.

+// return it.  Returns NULL if the key is not present or of the wrong type.
+// Is a cleaner implementation of base::mac::GetValueFromDictionary() above.

[Mark Mentovai, 2011/11/14 19:29:14]

This is a fragment, not a sentence. But if you’re be marking the other one as
deprecated and removing it, you don’t need this line at all.

+BASE_EXPORT template<typename T>
+T GetValueFromDictionary(CFDictionaryRef dict, CFStringRef key) {
+  CFTypeRef value = CFDictionaryGetValue(dict, key);
+  T value_specific = CFCast<T>(value);
+
+  if (value && !value_specific) {
+    std::string expected_type_ref = TypeNameForCFType(value_specific);
+    DLOG(WARNING) << GetValueFromDictionaryErrorMessage(key,
+                                                        expected_type_ref,
+                                                        value);
+  }
+  return value_specific;

[Mark Mentovai, 2011/11/14 19:29:14]

Add a blank line before this because this is conceptually a new “paragraph.”

+}
+
 }  // namespace mac
 }  // namespace base