Code changes for building on ARM

Some small changes to get chromium building on ARM. I tested these using the standard Linux tool chain and crosstools-ng.

[sky, 2009-05-04 17:09:11]

http://codereview.chromium.org/99349/diff/1/3
File base/debug_util_posix.cc (right):

http://codereview.chromium.org/99349/diff/1/3#newcode110
Line 110: #if !defined(ARM)
All our defines should be in build_config, and architecture level ones are
prefixed with ARCH_CPU. It looks like we already have ARCH_CPU_ARM_FAMILY. You
should use that here instead of ARM.

http://codereview.chromium.org/99349/diff/1/2
File base/string_util_unittest.cc (right):

http://codereview.chromium.org/99349/diff/1/2#newcode315
Line 315: EXPECT_EQ(0U, converted[0]);
Why does ARM treat a wchar_t as unsigned? Seems like that is going to require a
painful change to Chrome and the unit tests to support.

[Lei Zhang, 2009-05-04 17:58:05]

http://codereview.chromium.org/99349/diff/1/2
File base/string_util_unittest.cc (right):

http://codereview.chromium.org/99349/diff/1/2#newcode315
Line 315: EXPECT_EQ(0U, converted[0]);
On 2009/05/04 17:09:11, sky wrote:
> Why does ARM treat a wchar_t as unsigned? Seems like that is going to require
a
> painful change to Chrome and the unit tests to support.

I'm not sure why, but that's what gcc on ARM does. I've cross-compiled
everything and this is the only place where it's been an issue.

[sky, 2009-05-04 18:16:22]

On 2009/05/04 17:58:05, Lei Zhang wrote:
> http://codereview.chromium.org/99349/diff/1/2
> File base/string_util_unittest.cc (right):
> 
> http://codereview.chromium.org/99349/diff/1/2#newcode315
> Line 315: EXPECT_EQ(0U, converted[0]);
> On 2009/05/04 17:09:11, sky wrote:
> > Why does ARM treat a wchar_t as unsigned? Seems like that is going to
require
> a
> > painful change to Chrome and the unit tests to support.
> 
> I'm not sure why, but that's what gcc on ARM does. I've cross-compiled
> everything and this is the only place where it's been an issue.

Weird. I'm going to follow up on a list to all to see if anyone has an idea why
this is happening.

[sky, 2009-05-04 19:15:29]

http://codereview.chromium.org/99349/diff/1/2
File base/string_util_unittest.cc (right):

http://codereview.chromium.org/99349/diff/1/2#newcode315
Line 315: EXPECT_EQ(0U, converted[0]);
On 2009/05/04 17:58:05, Lei Zhang wrote:
> On 2009/05/04 17:09:11, sky wrote:
> > Why does ARM treat a wchar_t as unsigned? Seems like that is going to
require
> a
> > painful change to Chrome and the unit tests to support.
> 
> I'm not sure why, but that's what gcc on ARM does. I've cross-compiled
> everything and this is the only place where it's been an issue.

Ok, seems this is the way it's going to be.
Rather than #if defined(ARCH_CPU_ARM_FAMILY), should we have a switch like #if
defined(WCHAR_IS_SIGNED) and set it in build_config for arm?

[sky, 2009-05-04 19:57:37]

LGTM.

I'll fix the nit and land shortly.

http://codereview.chromium.org/99349/diff/8/1008
File base/string_util_unittest.cc (right):

http://codereview.chromium.org/99349/diff/8/1008#newcode314
Line 314: # if defined(WCHAR_T_IS_UNSIGNED)
nit: no space between # and if.

[Lei Zhang, 2009-05-04 20:15:04]

On 2009/05/04 19:57:37, sky wrote:
> LGTM.
> 
> I'll fix the nit and land shortly.
> 
> http://codereview.chromium.org/99349/diff/8/1008
> File base/string_util_unittest.cc (right):
> 
> http://codereview.chromium.org/99349/diff/8/1008#newcode314
> Line 314: # if defined(WCHAR_T_IS_UNSIGNED)
> nit: no space between # and if.

Can we write it as static_cast<wchar_t>(0) and get rid of the #if define ?

[sky, 2009-05-04 20:30:09]

On 2009/05/04 20:15:04, Lei Zhang wrote:
> On 2009/05/04 19:57:37, sky wrote:
> > LGTM.
> > 
> > I'll fix the nit and land shortly.
> > 
> > http://codereview.chromium.org/99349/diff/8/1008
> > File base/string_util_unittest.cc (right):
> > 
> > http://codereview.chromium.org/99349/diff/8/1008#newcode314
> > Line 314: # if defined(WCHAR_T_IS_UNSIGNED)
> > nit: no space between # and if.
> 
> Can we write it as static_cast<wchar_t>(0) and get rid of the #if define ?

I suspect that would easily be removed. And until we have ARM builds it would go
unnoticed.

[Lei Zhang, 2009-05-05 17:42:05]

LGTM

Ok, hopefully we'll have a bot at some point in the future.

[Evan Stade, 2009-05-06 07:20:25]

> Can we write it as static_cast<wchar_t>(0) and get rid of the #if define ?

couldn't you just: L'\0'

[Mark Mentovai, 2009-05-06 16:14:16]

The ugly code rescue squad has arrived.

http://codereview.chromium.org/99349/diff/1/2
File base/string_util_unittest.cc (right):

http://codereview.chromium.org/99349/diff/1/2#newcode315
Line 315: EXPECT_EQ(0U, converted[0]);
314 # if defined(ARM)
315   EXPECT_EQ(0U, converted[0]);
316 # else
317   EXPECT_EQ(0, converted[0]);
318 #endif

(And what's with the space between the # and the "if" and "else," anyway?)

sky wrote:
> Ok, seems this is the way it's going to be.
> Rather than #if defined(ARCH_CPU_ARM_FAMILY), should we have a switch like #if
> defined(WCHAR_IS_SIGNED) and set it in build_config for arm?

It's unfortunate that EXPECT_EQ works this way.  This would be valid whether
wchar_t is signed or unsigned:

  0 == converted[0]

I absolutely hate peppering tests with #ifdefs for cases like this.  As it
happens, there's a much better way to do exactly what you're trying to do here,
and strictly speaking, it's more correct:

  EXPECT_EQ(L'\0', converted[0])

Ideally, wchar_t character constants would always be written as L'', so lines
319, 320, and 325 should get this treatment as well.  In practice, you wind up
relying on char-to-wchar_t promotion; given how EXPECT_EQ works, as long as
wchar_t and char are of the same signedness, L'\0' and '\0' should be
equivalent.

In general, use '' in preference to numbers for character constants, even in the
case of '\0' vs. 0.  See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=0_and_...
.

[Mark Mentovai, 2009-05-06 16:18:16]

http://codereview.chromium.org/99349/diff/8/1009
File base/debug_util_posix.cc (right):

http://codereview.chromium.org/99349/diff/8/1009#newcode110
Line 110: #if !defined(ARCH_CPU_ARM_FAMILY)
This needs a TODO for someone to implement it on ARM, or an #else
NOTIMPLEMENTED().

http://codereview.chromium.org/99349/diff/8/1011
File build/build_config.h (right):

http://codereview.chromium.org/99349/diff/8/1011#newcode59
Line 59: #define WCHAR_T_IS_UNSIGNED 1
This is now of dubious value given my previous e-mail.

http://codereview.chromium.org/99349/diff/8/1010
File media/base/yuv_convert.cc (right):

http://codereview.chromium.org/99349/diff/8/1010#newcode76
Line 76: // MMX for Windows, Linux and Mac.
This comment should say "on x86".

http://codereview.chromium.org/99349/diff/8/1010#newcode81
Line 81: #ifdef ARCH_CPU_ARM_FAMILY
I think that this should be flipped, right?  We want "no MMX" on any non-x86,
not just on ARM.  So...

#ifdef ARCH_CPU_X86_FAMILY
#define USE_MMX 1
#else
#define USE_MMX 0
#endif

Right?