Make unit_tests pass with pt_BR.UTF-8 locale.

Make unit_tests pass with pt_BR.UTF-8 locale.

BUG=3675

[Mark Mentovai, 2008-11-07 18:00:50]

Do we need this for anything other than tests?  Don't we have ICU for
locale-driven things?

http://codereview.chromium.org/9509/diff/1/4
File base/json_writer.cc (right):

http://codereview.chromium.org/9509/diff/1/4#newcode10
Line 10: #include <xlocale.h>
Seems odd that Linux and Windows would use one header and Mac would use another.
 Can we just include one and have it be right everywhere?  Can we include both?

http://codereview.chromium.org/9509/diff/1/4#newcode75
Line 75: #else  // This means Windows.
#elif defined(OS_WINDOWS)

http://codereview.chromium.org/9509/diff/1/4#newcode76
Line 76: {
What's with the {scoping}?

Actually, the {scoping} is a good idea if we had some sort of stack-based object
to handle this and reset the locale settings when it goes out of scope.  We
could hide the differences between platforms in such an object.  What do you
think?

http://codereview.chromium.org/9509/diff/1/4#newcode79
Line 79: setlocate(LC_NUMERIC, "C");
You didn't test this on Windows, did you?

(It would be helpful when you send patches to let us know which platforms you
weren't able to test on - when you write things like #if defined(OS_MACOSX) and
have Windows code like this, I think people tend to assume you've tested on
these platforms.)

http://codereview.chromium.org/9509/diff/1/5
File base/string_util.cc (right):

http://codereview.chromium.org/9509/diff/1/5#newcode12
Line 12: #include <locale.h>
Again, we do have a locale.h on the Mac, so I don't know what this is about.

http://codereview.chromium.org/9509/diff/1/5#newcode23
Line 23: #include <xlocale.h>
And you do have an xlocale.h on Linux...

(...but not Windows.)

http://codereview.chromium.org/9509/diff/1/3
File base/string_util.h (right):

http://codereview.chromium.org/9509/diff/1/3#newcode130
Line 130: // When some function takes an optional parameter to control locale
independence
Comma after independence (or dependence).

http://codereview.chromium.org/9509/diff/1/3#newcode133
Line 133: LOCALE_DEPENDENT = false,
I think this enum would make more sense to me if it were LocaleDependence, and
LOCALE_DEPENDENT were the "true" value.

http://codereview.chromium.org/9509/diff/1/3#newcode387
Line 387: LocaleIndependence locale_independence=LOCALE_DEPENDENT);
Default arguments are forbidden by our style guide.

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

http://codereview.chromium.org/9509/diff/1/3#newcode389
Line 389: LocaleIndependence locale_independence=LOCALE_DEPENDENT);
I think the args should be named locale_independent (or locale_dependent). 
Makes the code read more fluidly.

[Evan Martin, 2008-11-07 19:30:22]

I kinda wish we wouldn't need to change locales at all.  Dean's supposedly gonna
show up in ~3 hours, so maybe he'll have some ideas.

[please use my chromium address, 2008-11-07 19:59:44]

On 2008/11/07 18:00:50, Mark Mentovai wrote:
> Do we need this for anything other than tests?

Yes. This is used at least by regular JSON routines, which would break in other
locales.

> Don't we have ICU for locale-driven things?

I noticed it, but thought it's for multibyte things. If needed, relevant part of
this changeset can be easily moved to string_util_icu.

> http://codereview.chromium.org/9509/diff/1/4
> File base/json_writer.cc (right):
> 
> http://codereview.chromium.org/9509/diff/1/4#newcode10
> Line 10: #include <xlocale.h>
> Seems odd that Linux and Windows would use one header and Mac would use
another.
>  Can we just include one and have it be right everywhere?  Can we include
both?

Done.

> http://codereview.chromium.org/9509/diff/1/4#newcode75
> Line 75: #else  // This means Windows.
> #elif defined(OS_WINDOWS)

Done.

> http://codereview.chromium.org/9509/diff/1/4#newcode76
> Line 76: {
> What's with the {scoping}?
> 
> Actually, the {scoping} is a good idea if we had some sort of stack-based
object
> to handle this and reset the locale settings when it goes out of scope.  We
> could hide the differences between platforms in such an object.  What do you
> think?

I actually thought about such class. This update includes my proposed
implementation. Of course it would be better not to change locale at all, even
temporarily, and even for one thread only. To do that, a function should be
added that would convert double to a string in a locale independent way.

> http://codereview.chromium.org/9509/diff/1/4#newcode79
> Line 79: setlocate(LC_NUMERIC, "C");
> You didn't test this on Windows, did you?
> 
> (It would be helpful when you send patches to let us know which platforms you
> weren't able to test on - when you write things like #if defined(OS_MACOSX)
and
> have Windows code like this, I think people tend to assume you've tested on
> these platforms.)

Oops. Indeed, I didn't test. But I'll include notices about that from now on.

> http://codereview.chromium.org/9509/diff/1/5#newcode23
> Line 23: #include <xlocale.h>
> And you do have an xlocale.h on Linux...
> 
> (...but not Windows.)

Please check if my way to handle this is right.

> http://codereview.chromium.org/9509/diff/1/3
> File base/string_util.h (right):
> 
> http://codereview.chromium.org/9509/diff/1/3#newcode130
> Line 130: // When some function takes an optional parameter to control locale
> independence
> Comma after independence (or dependence).

Done.

> http://codereview.chromium.org/9509/diff/1/3#newcode133
> Line 133: LOCALE_DEPENDENT = false,
> I think this enum would make more sense to me if it were LocaleDependence, and
> LOCALE_DEPENDENT were the "true" value.

Done. Indeed, seems better.

> http://codereview.chromium.org/9509/diff/1/3#newcode387
> Line 387: LocaleIndependence locale_independence=LOCALE_DEPENDENT);
> Default arguments are forbidden by our style guide.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul...

Fixed.

> http://codereview.chromium.org/9509/diff/1/3#newcode389
> Line 389: LocaleIndependence locale_independence=LOCALE_DEPENDENT);
> I think the args should be named locale_independent (or locale_dependent). 
> Makes the code read more fluidly.

Done.

[Evan Martin, 2008-11-10 22:23:40]

+dean, in case he has wisdom

[tony, 2008-11-10 23:19:08]

After discussion with Evan and Dean, two alternate ideas were suggested:

1) Use David Gay's dtoa (and also atod).  This would involve bringing the code
into third_party and having it be a dependency of base.  Lots of work, but
probably the right thing to do because it's written for parsers.

2) If option 1 seems like too much work right now, just use ICU to print the
number.  We already have ICU and you can make a locally scoped ICU Locale class
rather than having to override the chrome global Locale.  This sounds similar to
what you're doing with locale_util.h.  Maybe write a TODO about using David
Gay's code?

[please use my chromium address, 2008-11-11 15:05:58]

On 2008/11/10 23:19:08, tony wrote:
> After discussion with Evan and Dean, two alternate ideas were suggested:
> 
> 1) Use David Gay's dtoa (and also atod).  This would involve bringing the code
> into third_party and having it be a dependency of base.  Lots of work, but
> probably the right thing to do because it's written for parsers.

Well, I chose the first option. It compiles and works on Linux (might have
missed something because I also have other changes in gcl - of course I tried to
check it, but mistakes happen). I didn't test on Windows and Mac. I don't know
how dmg_fp should be added to their build system etc.

[Dean McNamee, 2008-11-11 18:50:17]

http://codereview.chromium.org/9509/diff/213/418
File base/string_util.cc (right):

http://codereview.chromium.org/9509/diff/213/418#newcode94
Line 94: LocaleDependence locale_dependent=LOCALE_DEPENDENT) {
We're not supposed to use default arguments.  You can just create a 2 parameter
version that calls the 3 parameter version :\

http://codereview.chromium.org/9509/diff/213/420
File third_party/dmg_fp/dmg_fp.scons (right):

http://codereview.chromium.org/9509/diff/213/420#newcode8
Line 8: )
All on one line?

[tony, 2008-11-11 18:55:00]

LGTM.  This is a really good patch.  Just some small style nits.  This'll take
some work to get to work on windows and mac.  I'll try to do that later today.

http://codereview.chromium.org/9509/diff/213/418
File base/string_util.cc (right):

http://codereview.chromium.org/9509/diff/213/418#newcode26
Line 26: 
Nit: no newline needed here.

http://codereview.chromium.org/9509/diff/213/416
File base/string_util.h (right):

http://codereview.chromium.org/9509/diff/213/416#newcode130
Line 130: // When some function takes an optional parameter to control locale
independence,
Nit: This is over 80 col.

http://codereview.chromium.org/9509/diff/213/416#newcode133
Line 133: LOCALE_DEPENDENT = true,
Nit: = true and = false aren't really necessary.

[please use my chromium address, 2008-11-11 21:26:35]

Thanks for testing. I fixed all issues before the one below.

> http://codereview.chromium.org/9509/diff/213/416#newcode133
> Line 133: LOCALE_DEPENDENT = true,
> Nit: = true and = false aren't really necessary.

I find it useful to use it as a boolean, like a parameter called
locale_dependent and then if (locale_dependent). Would it behave the same with
"= true" and "= false" removed? If you wish, I can just replace the boolean
checks with explicit checks (locale_dependent == LOCALE_DEPENDENT).

[tony, 2008-11-11 21:41:31]

Explicit checks sound good to me.

On 2008/11/11 21:26:35, phajdan.jr wrote:
> Thanks for testing. I fixed all issues before the one below.
> 
> > http://codereview.chromium.org/9509/diff/213/416#newcode133
> > Line 133: LOCALE_DEPENDENT = true,
> > Nit: = true and = false aren't really necessary.
> 
> I find it useful to use it as a boolean, like a parameter called
> locale_dependent and then if (locale_dependent). Would it behave the same with
> "= true" and "= false" removed? If you wish, I can just replace the boolean
> checks with explicit checks (locale_dependent == LOCALE_DEPENDENT).

[tony, 2008-11-12 01:15:36]

I went ahead and made the change and did the windows necessary changes.  You can
see the work so far in 

http://codereview.chromium.org/11002

I'll try to update the xcode files next.


On 2008/11/11 21:41:31, tony wrote:
> Explicit checks sound good to me.
> 
> On 2008/11/11 21:26:35, phajdan.jr wrote:
> > Thanks for testing. I fixed all issues before the one below.
> > 
> > > http://codereview.chromium.org/9509/diff/213/416#newcode133
> > > Line 133: LOCALE_DEPENDENT = true,
> > > Nit: = true and = false aren't really necessary.
> > 
> > I find it useful to use it as a boolean, like a parameter called
> > locale_dependent and then if (locale_dependent). Would it behave the same
with
> > "= true" and "= false" removed? If you wish, I can just replace the boolean
> > checks with explicit checks (locale_dependent == LOCALE_DEPENDENT).

[tony, 2008-11-12 23:21:04]

Committed in r5319. Let's hope it works.