Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2717613005:
[intl] Fix NumberFormat options handling spec compliance issues (Closed)
|
Created:
3 years, 10 months ago by Dan Ehrenberg Modified:
3 years, 8 months ago CC:
v8-reviews_googlegroups.com, Yang Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
Description[intl] Fix NumberFormat options handling spec compliance issues
The goal of this patch was to refactor NumberFormat parameter handling
to be usable by a PluralRules implementation. Along the way, I found
and fixed a couple minor issues where options handling differed from
the specification, and removed some dead code. Regression tests are
added as test262 tests. With this change, the overall flow more closely
resembles the specification plus this editorial change which is out
for review: https://github.com/tc39/ecma402/pull/130/files
BUG=v8:6015, v8:6016
R=yangguo,jungshik
Review-Url: https://codereview.chromium.org/2717613005
Cr-Commit-Position: refs/heads/master@{#44571}
Committed: https://chromium.googlesource.com/v8/v8/+/38c5394ccb50a9e87cda5ab297fba34a36387276
Patch Set 1 #Patch Set 2 : Fix issues #Patch Set 3 : Slight cleanup #
Total comments: 1
Patch Set 4 : Rebase #Patch Set 5 : Run the local test262 tests #
Total comments: 2
Patch Set 6 : Rebase and fix nit #
Total comments: 5
Patch Set 7 : Fix rebase #Patch Set 8 : Add a TODO #Patch Set 9 : Rebase #Patch Set 10 : Remove test262 failure expectation line #
Messages
Total messages: 55 (42 generated)
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [intl] Fix NumberFormat options handling spec compliance issues The goal of this patch was to refactor NumberFormat parameter handling to be usable by a PluralRules implementation. Along the way, I found and fixed a couple minor issues where options handling differed from the specification, and removed some dead code. BUG=v8:6015,v8:6016 R=yangguo,jungshik ========== to ========== [intl] Fix NumberFormat options handling spec compliance issues The goal of this patch was to refactor NumberFormat parameter handling to be usable by a PluralRules implementation. Along the way, I found and fixed a couple minor issues where options handling differed from the specification, and removed some dead code. Regression tests are added as test262 tests. BUG=v8:6015,v8:6016 R=yangguo,jungshik ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/18044)
Description was changed from ========== [intl] Fix NumberFormat options handling spec compliance issues The goal of this patch was to refactor NumberFormat parameter handling to be usable by a PluralRules implementation. Along the way, I found and fixed a couple minor issues where options handling differed from the specification, and removed some dead code. Regression tests are added as test262 tests. BUG=v8:6015,v8:6016 R=yangguo,jungshik ========== to ========== [intl] Fix NumberFormat options handling spec compliance issues The goal of this patch was to refactor NumberFormat parameter handling to be usable by a PluralRules implementation. Along the way, I found and fixed a couple minor issues where options handling differed from the specification, and removed some dead code. Regression tests are added as test262 tests. With this change, the overall flow more closely resembles the specification plus this editorial change which is out for review: https://github.com/tc39/ecma402/pull/130/files BUG=v8:6015,v8:6016 R=yangguo,jungshik ==========
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2717613005/diff/40001/src/runtime/runtime-i18... File src/runtime/runtime-i18n.cc (right): https://codereview.chromium.org/2717613005/diff/40001/src/runtime/runtime-i18... src/runtime/runtime-i18n.cc:561: icu::UnicodeString::fromUTF8(*currency_string); Yes, this is a terrible way to get a UnicodeString from a V8 string, but this one needs to be null-terminated, and I'm following the way that's done in the rest of the file. A cleanup for all of these usage sites should come in a follow-on patch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
littledan@chromium.org changed reviewers: + yangguo@chromium.org - yangguo@google.com
adamk@chromium.org changed reviewers: + adamk@chromium.org, jshin@chromium.org - jungshik@google.com, yangguo@chromium.org
Code lgtm, but deferring to jshin@ on the i18n-specific behavior changes. https://codereview.chromium.org/2717613005/diff/100001/src/runtime/runtime-i1... File src/runtime/runtime-i18n.cc (right): https://codereview.chromium.org/2717613005/diff/100001/src/runtime/runtime-i1... src/runtime/runtime-i18n.cc:563: { Nit: no need for this block, since its ending brace is already at the end of the function.
Note that none of the changes here actually get at the real content of internationalization; they're all superficial things in the "frontend" governed by ordinary, clear spec text. https://codereview.chromium.org/2717613005/diff/100001/src/runtime/runtime-i1... File src/runtime/runtime-i18n.cc (right): https://codereview.chromium.org/2717613005/diff/100001/src/runtime/runtime-i1... src/runtime/runtime-i18n.cc:563: { On 2017/03/16 22:47:49, adamk wrote: > Nit: no need for this block, since its ending brace is already at the end of the > function. Fixed
Even aside from the spec, jshin's a better reviewer for the i18n.{cc,js} changes
just from code familiarity (at least for now; happy to follow along).
On 2017/03/17 16:58:32, adamk wrote:
> Even aside from the spec, jshin's a better reviewer for the i18n.{cc,js}
changes
> just from code familiarity (at least for now; happy to follow along).
Sorry for the delay. I'll get to this CL soon. (I'm swamped atm).
LGTM with two typos below fixed. https://codereview.chromium.org/2717613005/diff/120001/src/runtime/runtime-i1... File src/runtime/runtime-i18n.cc (right): https://codereview.chromium.org/2717613005/diff/120001/src/runtime/runtime-i1... src/runtime/runtime-i18n.cc:560: v8::String::Utf8Value currency_string(v8::Utils::ToLocal(currency)); JFYI: I tried to avoid this pattern ( see lines 417 ~ 430 in the RHS of https://codereview.chromium.org/1991753002/diff/20001/src/runtime/runtime-i18... ), but I guess it does not matter much. https://codereview.chromium.org/2717613005/diff/120001/src/runtime/runtime-i1... src/runtime/runtime-i18n.cc:568: icu::toUCharPtr(currency.getTerminatedBuffer()), &status_digits); You meant currency_icu here? It seems that it's a typo from PS #5 => PS #6. PS #5 does not have this typo. :-) https://codereview.chromium.org/2717613005/diff/120001/src/runtime/runtime-i1... src/runtime/runtime-i18n.cc:571: currency.getTerminatedBuffer(), &status_digits); same here.. It's a pity that ucurr_getDefaultFractionDigits takes UChar* for something that is always ASCII. It could have just taken 'char*'. Alternatively, if there were a C++ API, you'd not have to do U_ICU_VERSION... either.
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2717613005/diff/120001/src/runtime/runtime-i1... File src/runtime/runtime-i18n.cc (right): https://codereview.chromium.org/2717613005/diff/120001/src/runtime/runtime-i1... src/runtime/runtime-i18n.cc:560: v8::String::Utf8Value currency_string(v8::Utils::ToLocal(currency)); On 2017/03/21 20:29:34, jungshik at Google wrote: > JFYI: I tried to avoid this pattern ( see lines 417 ~ 430 in the RHS of > https://codereview.chromium.org/1991753002/diff/20001/src/runtime/runtime-i18... > ), but I guess it does not matter much. What's the right way to avoid it when constructing a UChar* ? If the input is a char *, should I be passing in a Latin1 UConverter to the UnicodeString converter? I added a TODO here. Mind if it's left for a future patch? https://codereview.chromium.org/2717613005/diff/120001/src/runtime/runtime-i1... src/runtime/runtime-i18n.cc:568: icu::toUCharPtr(currency.getTerminatedBuffer()), &status_digits); On 2017/03/21 20:29:35, jungshik at Google wrote: > You meant currency_icu here? It seems that it's a typo from PS #5 => PS #6. PS > #5 does not have this typo. :-) Oops, fixed
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by littledan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org, jshin@chromium.org Link to the patchset: https://codereview.chromium.org/2717613005/#ps160001 (title: "Add a TODO")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/25878) v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)
The CQ bit was checked by littledan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jshin@chromium.org, adamk@chromium.org Link to the patchset: https://codereview.chromium.org/2717613005/#ps200001 (title: "Remove test262 failure expectation line")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1491912268645840,
"parent_rev": "7deb682187d223cddeb57402f3bb335cfa1d6bb6", "commit_rev":
"38c5394ccb50a9e87cda5ab297fba34a36387276"}
Message was sent while issue was closed.
Description was changed from ========== [intl] Fix NumberFormat options handling spec compliance issues The goal of this patch was to refactor NumberFormat parameter handling to be usable by a PluralRules implementation. Along the way, I found and fixed a couple minor issues where options handling differed from the specification, and removed some dead code. Regression tests are added as test262 tests. With this change, the overall flow more closely resembles the specification plus this editorial change which is out for review: https://github.com/tc39/ecma402/pull/130/files BUG=v8:6015,v8:6016 R=yangguo,jungshik ========== to ========== [intl] Fix NumberFormat options handling spec compliance issues The goal of this patch was to refactor NumberFormat parameter handling to be usable by a PluralRules implementation. Along the way, I found and fixed a couple minor issues where options handling differed from the specification, and removed some dead code. Regression tests are added as test262 tests. With this change, the overall flow more closely resembles the specification plus this editorial change which is out for review: https://github.com/tc39/ecma402/pull/130/files BUG=v8:6015,v8:6016 R=yangguo,jungshik Review-Url: https://codereview.chromium.org/2717613005 Cr-Commit-Position: refs/heads/master@{#44571} Committed: https://chromium.googlesource.com/v8/v8/+/38c5394ccb50a9e87cda5ab297fba34a363... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001) as https://chromium.googlesource.com/v8/v8/+/38c5394ccb50a9e87cda5ab297fba34a363... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
