Remove wchar_t versions of StringPrintf from Android

base/stringprintf.h

Index: base/stringprintf.h
diff --git a/base/stringprintf.h b/base/stringprintf.h
index 40ad107aead14fd3af62aeafb63f6aae21ac861a..9f2463b2dd7f62399c8e6bfa54c0aeb992c76144 100644
--- a/base/stringprintf.h
+++ b/base/stringprintf.h
@@ -17,9 +17,6 @@ namespace base {
 // Return a C++ string given printf-like input.
 BASE_EXPORT std::string StringPrintf(const char* format, ...)
     PRINTF_FORMAT(1, 2);
-BASE_EXPORT std::wstring StringPrintf(const wchar_t* format, ...)
-    WPRINTF_FORMAT(1, 2);
-
 // Return a C++ string given vprintf-like input.
 BASE_EXPORT std::string StringPrintV(const char* format, va_list ap)
     PRINTF_FORMAT(1, 0);
@@ -28,25 +25,34 @@ BASE_EXPORT std::string StringPrintV(const char* format, va_list ap)
 BASE_EXPORT const std::string& SStringPrintf(std::string* dst,
                                              const char* format, ...)
     PRINTF_FORMAT(2, 3);
-BASE_EXPORT const std::wstring& SStringPrintf(std::wstring* dst,
-                                              const wchar_t* format, ...)
-    WPRINTF_FORMAT(2, 3);
 
 // Append result to a supplied string.
 BASE_EXPORT void StringAppendF(std::string* dst, const char* format, ...)

[jar (doing other things), 2012/07/24 18:15:34]

This is interesting.... it was previously only advertised for non-android, and
now you're going to advertise and presumably use it.

Where are each of these used in this CL that required making them visible?

...and if you want them visible... I'd rather use separate ifdefs around the
sibling items (like line 40-41) that you want to hid.

[Iain Merrick, 2012/07/25 10:57:23]

Hmm, probably more interesting than I intended! I'll clean up with separate
ifdefs.

     PRINTF_FORMAT(2, 3);
-// TODO(evanm): this is only used in a few places in the code;
-// replace with string16 version.
-BASE_EXPORT void StringAppendF(std::wstring* dst, const wchar_t* format, ...)
-    WPRINTF_FORMAT(2, 3);
 
 // Lower-level routine that takes a va_list and appends to a specified
 // string.  All other routines are just convenience wrappers around it.
 BASE_EXPORT void StringAppendV(std::string* dst, const char* format, va_list ap)
     PRINTF_FORMAT(2, 0);
+
+// wchar_t variants of above functions. Not supported by Android's libc.
+#if !defined(OS_ANDROID)

[jar (doing other things), 2012/07/24 18:15:34]

IMO, this grouping (by whether we're Android or not) to reduce the number of #if
statements, makes readability worse.

I don't like #if stuff (so I can sympathize with the motivation), but with
header files, folks expect to find similar functionality (e.g., StringPrintf()
for various flavors of strings) to be near either other.  I wouldn't expect to
have to look in a special non-android section to find a function :-/.

As a result, I'd rather leave the functions mostly where they naturally landed,
and put in the separate sets of #if statements around them.

[Iain Merrick, 2012/07/25 10:57:23]

Sure, no problem! I considered both and made the opposite call, but I'm happy to
defer to your decision. :)

I also considered adding a special macro -- HAVE_WPRINTF or some such. Let me
know if you think that's a good idea.

[jar (doing other things), 2012/07/25 18:54:04]

Mixed feelings:

Macros are frowned upon.  If you wanted to add one, you'd have to be REAL
careful to avoid conflicting with others, and plausibly with conventions(?).

Your suggestion would make this header more readable.

IMO, risks of problems via name pollution outweigh the gain.

I'd suggest adding one comment, before your first use of the ifdef here, saying
that: 

"OS_ANDROID does not have wchar_t, so several overloads are omitted"


On 2012/07/25 10:57:23, Iain Merrick wrote:

[Iain Merrick, 2012/07/26 11:15:56]

Done.

+BASE_EXPORT std::wstring StringPrintf(const wchar_t* format, ...)
+    WPRINTF_FORMAT(1, 2);
+
+BASE_EXPORT const std::wstring& SStringPrintf(std::wstring* dst,
+                                              const wchar_t* format, ...)
+    WPRINTF_FORMAT(2, 3);
+
+// TODO(evanm): this is only used in a few places in the code;
+// replace with string16 version.
+BASE_EXPORT void StringAppendF(std::wstring* dst, const wchar_t* format, ...)
+    WPRINTF_FORMAT(2, 3);
+
 BASE_EXPORT void StringAppendV(std::wstring* dst,
                                const wchar_t* format, va_list ap)
     WPRINTF_FORMAT(2, 0);
+#endif
 
 }  // namespace base