Upstream android string implementation etc.

Upstream android string implementation etc.


BUG=
TEST=

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

[darin (slow to review), 2011-06-24 16:46:05]

http://codereview.chromium.org/7238018/diff/1006/base/hash_tables.h
File base/hash_tables.h (right):

http://codereview.chromium.org/7238018/diff/1006/base/hash_tables.h#newcode38
base/hash_tables.h:38: #define HASH_NAMESPACE std
since this is a global #define, you should probably prefix it with BASE_.  that
way, you decrease chances of collisions with consumers.

also, it might be good to define this on all platforms to minimize the chances
of someone expecting it to be there on windows, but then finding out only after
submitting a try job that it is not.

again, since you are adding a new global #define, it is good to be careful.

http://codereview.chromium.org/7238018/diff/1006/base/string_number_conversio...
File base/string_number_conversions.cc (right):

http://codereview.chromium.org/7238018/diff/1006/base/string_number_conversio...
base/string_number_conversions.cc:406: #if !defined(OS_ANDROID)
i think it would be better to declare a #define like
STD_STRING_ITERATOR_IS_CHAR_POINTER.  that way you do not need to duplicate the
comment everywhere.

this reminds me of how wchar_t == char16 when compiling under MSVC.  so we have
the WCHAR_T_IS_UTF16 #define, which we use to avoid a similar warning on MSVC. 
on other platforms, wchar_t is 32-bits.

STD_STRING_ITERATOR_IS_CHAR_POINTER would probably get added to
build/build_config.h.

[michaelbai, 2011-06-24 18:51:29]

Addressed comments

http://codereview.chromium.org/7238018/diff/1006/base/hash_tables.h
File base/hash_tables.h (right):

http://codereview.chromium.org/7238018/diff/1006/base/hash_tables.h#newcode38
base/hash_tables.h:38: #define HASH_NAMESPACE std
On 2011/06/24 16:46:05, darin wrote:
> since this is a global #define, you should probably prefix it with BASE_. 
that
> way, you decrease chances of collisions with consumers.
> 
> also, it might be good to define this on all platforms to minimize the chances
> of someone expecting it to be there on windows, but then finding out only
after
> submitting a try job that it is not.
> 
> again, since you are adding a new global #define, it is good to be careful.

Done.

http://codereview.chromium.org/7238018/diff/1006/base/string_number_conversio...
File base/string_number_conversions.cc (right):

http://codereview.chromium.org/7238018/diff/1006/base/string_number_conversio...
base/string_number_conversions.cc:406: #if !defined(OS_ANDROID)
On 2011/06/24 16:46:05, darin wrote:
> i think it would be better to declare a #define like
> STD_STRING_ITERATOR_IS_CHAR_POINTER.  that way you do not need to duplicate
the
> comment everywhere.
> 
> this reminds me of how wchar_t == char16 when compiling under MSVC.  so we
have
> the WCHAR_T_IS_UTF16 #define, which we use to avoid a similar warning on MSVC.

> on other platforms, wchar_t is 32-bits.
> 
> STD_STRING_ITERATOR_IS_CHAR_POINTER would probably get added to
> build/build_config.h.

Thanks, Done.

[darin (slow to review), 2011-06-24 23:41:12]

OK, just one more thing...

http://codereview.chromium.org/7238018/diff/2001/base/hash_tables.h
File base/hash_tables.h (right):

http://codereview.chromium.org/7238018/diff/2001/base/hash_tables.h#newcode69
base/hash_tables.h:69: namespace base {
i guess it would be nice to not repeat this code.  maybe you can just move this
down to the end of the file?

[michaelbai, 2011-06-24 23:45:35]

http://codereview.chromium.org/7238018/diff/2001/base/hash_tables.h
File base/hash_tables.h (right):

http://codereview.chromium.org/7238018/diff/2001/base/hash_tables.h#newcode69
base/hash_tables.h:69: namespace base {
On 2011/06/24 23:41:12, darin wrote:
> i guess it would be nice to not repeat this code.  maybe you can just move
this
> down to the end of the file?

I thought about it before, the whole structure in this file like

#if defined(COMPILER_MSVC)

#elif defined(COMPILER_GCC)

#endif

There is no #else

Is it ok for me to add a #else like

#else
#define BASE_HASH_NAMESPACE
#endif

[brettw, 2011-06-24 23:57:04]

http://codereview.chromium.org/7238018/diff/2001/base/hash_tables.h
File base/hash_tables.h (right):

http://codereview.chromium.org/7238018/diff/2001/base/hash_tables.h#newcode39
base/hash_tables.h:39: #elif defined(COMPILER_GCC)
CAn you add blank lines around this for readability now that the ifdefs are
getting more complicated?

http://codereview.chromium.org/7238018/diff/2001/base/hash_tables.h#newcode69
base/hash_tables.h:69: namespace base {
Do we support any other compilers? In these cases we've often done:
#else
#error define BASE_HASH_NAMESPACE for your compiler
#endif

[michaelbai, 2011-06-27 16:14:09]

http://codereview.chromium.org/7238018/diff/2001/base/hash_tables.h
File base/hash_tables.h (right):

http://codereview.chromium.org/7238018/diff/2001/base/hash_tables.h#newcode39
base/hash_tables.h:39: #elif defined(COMPILER_GCC)
On 2011/06/24 23:57:04, brettw wrote:
> CAn you add blank lines around this for readability now that the ifdefs are
> getting more complicated?

Done.

http://codereview.chromium.org/7238018/diff/2001/base/hash_tables.h#newcode69
base/hash_tables.h:69: namespace base {
On 2011/06/24 23:57:04, brettw wrote:
> Do we support any other compilers? In these cases we've often done:
> #else
> #error define BASE_HASH_NAMESPACE for your compiler
> #endif

Done.

[darin (slow to review), 2011-06-27 17:13:46]

LGTM