Adding support for number formating to the JS i18n API. (issue7129051)
ci...@chromium.org
Message:
Jungshink: please take a look at ICU calls and overall logic.
Mads:
I can't use internal methods so I had to make some interesting choices on
how to
detect NaN in C++ code.
Could you take a look at JSNumberFormat method where I eval('NaN') to get
actual
value and see if it makes sense.
From some forum posts it seems v8 uses 2 values for NaN (-NaN and NaN).
True?
Description:
Adding support for number formating to the JS i18n API.
This is the last part of the API that belongs in public spec.
Methods supported:
- format
- derive
Options supported:
- style (decimal, scientific, currency and percent)
- pattern
- skeleton
TEST= Visit i18n.kaziprst.org/numberformat.html
Please review this at http://codereview.chromium.org/7129051/
SVN Base: http://v8.googlecode.com/svn/branches/bleeding_edge/
Affected files:
M src/extensions/experimental/experimental.gyp
M src/extensions/experimental/i18n-extension.cc
M src/extensions/experimental/i18n.js
A src/extensions/experimental/number-format.h
A src/extensions/experimental/number-format.cc
ag...@chromium.org
anything. The rest of the V8 usage looks fine.
http://codereview.chromium.org/7129051/diff/3001/src/extensions/experimental/number-format.cc
File src/extensions/experimental/number-format.cc (right):
http://codereview.chromium.org/7129051/diff/3001/src/extensions/experimental/number-format.cc#newcode90
src/extensions/experimental/number-format.cc:90: if (value == nan_) {
This does not work. NaN is not equal to anything so this should always
be false. Have you tested this?
You do need a is_nan method here. Most platforms provide one directly
and windows has is_nan_. Can you introduce a i18n-extension-utils.h file
that does the right platform #ifdef magic to make a is_nan method
available on all platforms?
http://codereview.chromium.org/7129051/diff/3001/src/extensions/experimental/number-format.h
File src/extensions/experimental/number-format.h (right):
http://codereview.chromium.org/7129051/diff/3001/src/extensions/experimental/number-format.h#newcode58
src/extensions/experimental/number-format.h:58: void* param);
Indentation is a bit off.
ag...@chromium.org
give
you either "NaN" or some special value signaling that it was asked to
format a
NaN value? If it does that will simplify the problem because you let ICU
deal
with it and just switch on the result from the ICU formatting.
Nebojša Ćirić
This version of NaN handling is not going to work since NaN is not equal to
anything. The rest of the V8 usage looks fine.
http://codereview.chromium.org/7129051/diff/3001/src/extensions/experimental/number-format.cc
File src/extensions/experimental/number-format.cc (right):
http://codereview.chromium.org/7129051/diff/3001/src/extensions/experimental/number-format.cc#newcode90
src/extensions/experimental/number-format.cc:90: if (value == nan_) {
This does not work. NaN is not equal to anything so this should always
be false. Have you tested this?
You do need a is_nan method here. Most platforms provide one directly
and windows has is_nan_. Can you introduce a i18n-extension-utils.h file
that does the right platform #ifdef magic to make a is_nan method
available on all platforms?
http://codereview.chromium.org/7129051/diff/3001/src/extensions/experimental/number-format.h
File src/extensions/experimental/number-format.h (right):
http://codereview.chromium.org/7129051/diff/3001/src/extensions/experimental/number-format.h#newcode58
src/extensions/experimental/number-format.h:58: void* param);
Indentation is a bit off.
--
Nebojša Ćirić
Lasse R.H. Nielsen
>
>
> 10. јун 2011. 00.25, <ag...@chromium.org> је написао/ла:
>>
>> This version of NaN handling is not going to work since NaN is not equal
>> to
>> anything. The rest of the V8 usage looks fine.
>
> It seemed that NaN can take only 2 values in v8 world -
> see http://code.google.com/p/v8/issues/detail?id=1101
We don't make any promises, but yes, currently we only ever create a
SNaN with zero payload, possibly signed (simply because negation on
doubles doesn't check for NaN).
Don't rely on it unless there is a good reason, though. It's not
guaranteed to stay that way.
And if you use is_nan (which you must, because NaN != NaN, even if
it's the same NaN representation), it won't matter.
/L
--
Lasse R.H. Nielsen
l...@chromium.org
ci...@chromium.org
Not ready for another review.
I'll add currencyCode (to match update in the docs) to the settings and
then ask
for another round.
http://codereview.chromium.org/7129051/diff/3001/src/extensions/experimental/number-format.cc
File src/extensions/experimental/number-format.cc (right):
http://codereview.chromium.org/7129051/diff/3001/src/extensions/experimental/number-format.cc#newcode90
src/extensions/experimental/number-format.cc:90: if (value == nan_) {
ICU can handle NaN properly so I've removed hacky code.
On 2011/06/10 07:25:30, Mads Ager wrote:
> This does not work. NaN is not equal to anything so this should always
be false.
> Have you tested this?
> You do need a is_nan method here. Most platforms provide one directly
and
> windows has is_nan_. Can you introduce a i18n-extension-utils.h file
that does
> the right platform #ifdef magic to make a is_nan method available on
all
> platforms?
http://codereview.chromium.org/7129051/diff/3001/src/extensions/experimental/number-format.h
File src/extensions/experimental/number-format.h (right):
http://codereview.chromium.org/7129051/diff/3001/src/extensions/experimental/number-format.h#newcode58
src/extensions/experimental/number-format.h:58: void* param);
On 2011/06/10 07:25:30, Mads Ager wrote:
> Indentation is a bit off.
Done.
ci...@chromium.org
I did one more pass over the code to fix couple of skeleton issues.
I also added currencyCode setting in i18n.js. If present it will override
region
ID inferred currency code (in native code).
ag...@chromium.org
js...@chromium.org
http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental/i18n.js
File src/extensions/experimental/i18n.js (right):
http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental/i18n.js#newcode267
src/extensions/experimental/i18n.js:267:
/^[a-zA-Z]{3}$/.test(settings['currencyCode'])) {
Is it your intent to allow mixed-case 3-letter code?
http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental/i18n.js#newcode268
src/extensions/experimental/i18n.js:268: cleanSettings['currencyCode'] =
settings['currencyCode'].toUpperCase();
It looks like it's assumed that v8's toUpperCase will remain always
locale-independent (i.e. acting like POSIX locale : [a-z] => [A-Z]). At
least, add a comment about that.
http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental/number-format.cc
File src/extensions/experimental/number-format.cc (right):
http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental/number-format.cc#newcode220
src/extensions/experimental/number-format.cc:220: // It affects currency
formatters only.
Pls, add a comment that |region| here is actually a full-blown locale id
in the form of 'und_' + region ID. An alternative is: 1) do not prepend
'und_' on the JavaScript-side 2) add 'und_' at the last moment inside
GetCurrencyCode().
http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental/number-format.cc#newcode227
src/extensions/experimental/number-format.cc:227: }
What if the locale id (icu_locale) itself has a currency code specified
(with '-u-cu-FOO')? We have to determine the priority order among three
possible sources.
http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental/number-format.cc#newcode244
src/extensions/experimental/number-format.cc:244: // UChar
representation of \x00A4 currency symbol.
nit: \x00A4 => U+00A4
http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental/number-format.cc#newcode245
src/extensions/experimental/number-format.cc:245: const UChar
currency_symbol = 0xA4;
nit: Didn't any complier complain? Perhaps, it's safer to add 'u'.
http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental/number-format.cc#newcode249
src/extensions/experimental/number-format.cc:249: // Find how many 0xA4
are there. There is at least one.
nit: U+00A4
BTW, the code below assumes that a skeleton has one or more U+00A4's in
a row. What if there is an intervening character between them?
Shouldn't we use a regex matcher (to match 1 to 3 (or 4) U+00A4's in a
row? Actually, using regex seems to be an overkill. Perhaps, after
finding |end_index}, you can walk from index to end_index to make sure
there' no other character in-between. If there is, you can reject the
skeleton as invalid.
http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental/number-format.cc#newcode268
src/extensions/experimental/number-format.cc:268:
icu::NumberFormat::createPercentInstance(icu_locale, *status));
no support for scientific with a skeleton?
http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental/number-format.cc#newcode279
src/extensions/experimental/number-format.cc:279: // Copy important
information from skeleton to the new formatter.
I wonder if there's anything else to pass from skeleton. e.g.
roundingIncrement (http://goo.gl/RU4qG )?
roundingMode cannot be set via a pattern. So, we can't support in
skeleton, but roundingIncrement can be set via a pattern....
http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental/number-format.cc#newcode311
src/extensions/experimental/number-format.cc:311: // or provided region
id.
Here again, it's confusing to say 'region id' because it's more than
that. ('und_' + region ID).
http://codereview.chromium.org/7129051/diff/11004/src/extensions/experimental/number-format.cc#newcode326
src/extensions/experimental/number-format.cc:326: const char* region =
icu_locale.getName();
wrapping up 'und_' + regionID in Locale class only to get back the raw
string seems a bit roo round-about. I'd just pass around 'region ID'
alone. OTOH, having to prepend 'und_' with char array is not fun... So,
it's up to you.
Anyway, GetCurrencyCode() has to be revised to deal with '-u-cu-FOO'
case in icu_locale.