Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1501113002:
Set the Gregorian changeover date to the beginning of time in Intl (Closed)
|
Created:
5 years ago by Dan Ehrenberg Modified:
5 years ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionSet the Gregorian changeover date to the beginning of time in Intl
ECMAScript dates act as if the Gregorian changeover happened at the
beginning of time. This patch fixes up internationalized date
formatting to set that changeover properly, as opposed to the ICU
default which is in the 16th century.
BUG=chromium:537382
R=adamk,cira
LOG=Y
Committed: https://crrev.com/d67756a7753e322fdd986399677a45a0459a5d40
Cr-Commit-Position: refs/heads/master@{#32669}
Patch Set 1 #Patch Set 2 : fix typo #Patch Set 3 : Enable test262 test which passes now #Patch Set 4 : Fix status issue #Patch Set 5 : Enable another test262 test #
Total comments: 1
Patch Set 6 : Replace a CHECK with DCHECK #
Messages
Total messages: 32 (13 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/patch-status/1501113002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1501113002/1
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/patch-status/1501113002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1501113002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/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/patch-status/1501113002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1501113002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...) v8_linux_arm_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/11050)
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/patch-status/1501113002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1501113002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_arm_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/11116)
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/patch-status/1501113002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1501113002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1501113002/diff/80001/src/i18n.cc File src/i18n.cc (right): https://codereview.chromium.org/1501113002/diff/80001/src/i18n.cc#newcode107 src/i18n.cc:107: CHECK(U_SUCCESS(status)); Can you teach me a bit about ICU? A CHECK here looks a little scary. Can this just never fail?
On 2015/12/08 at 01:20:36, adamk wrote: > https://codereview.chromium.org/1501113002/diff/80001/src/i18n.cc > File src/i18n.cc (right): > > https://codereview.chromium.org/1501113002/diff/80001/src/i18n.cc#newcode107 > src/i18n.cc:107: CHECK(U_SUCCESS(status)); > Can you teach me a bit about ICU? A CHECK here looks a little scary. Can this just never fail? AFAICT it could only fail due to a failed memory allocation. What should I do in that case? I could make it a DCHECK and just give an incorrect answer for that edge case.
On 2015/12/08 01:23:57, Dan Ehrenberg wrote: > On 2015/12/08 at 01:20:36, adamk wrote: > > https://codereview.chromium.org/1501113002/diff/80001/src/i18n.cc > > File src/i18n.cc (right): > > > > https://codereview.chromium.org/1501113002/diff/80001/src/i18n.cc#newcode107 > > src/i18n.cc:107: CHECK(U_SUCCESS(status)); > > Can you teach me a bit about ICU? A CHECK here looks a little scary. Can this > just never fail? > > AFAICT it could only fail due to a failed memory allocation. What should I do in > that case? I could make it a DCHECK and just give an incorrect answer for that > edge case. DCHECK sounds better to me, without knowing anything about ICU.
On 2015/12/08 at 01:30:56, adamk wrote: > On 2015/12/08 01:23:57, Dan Ehrenberg wrote: > > On 2015/12/08 at 01:20:36, adamk wrote: > > > https://codereview.chromium.org/1501113002/diff/80001/src/i18n.cc > > > File src/i18n.cc (right): > > > > > > https://codereview.chromium.org/1501113002/diff/80001/src/i18n.cc#newcode107 > > > src/i18n.cc:107: CHECK(U_SUCCESS(status)); > > > Can you teach me a bit about ICU? A CHECK here looks a little scary. Can this > > just never fail? > > > > AFAICT it could only fail due to a failed memory allocation. What should I do in > > that case? I could make it a DCHECK and just give an incorrect answer for that > > edge case. > > DCHECK sounds better to me, without knowing anything about ICU. Done
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/patch-status/1501113002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1501113002/100001
lgtm
The CQ bit was unchecked by littledan@chromium.org
The CQ bit was checked by littledan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1501113002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1501113002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Set the Gregorian changeover date to the beginning of time in Intl ECMAScript dates act as if the Gregorian changeover happened at the beginning of time. This patch fixes up internationalized date formatting to set that changeover properly, as opposed to the ICU default which is in the 16th century. BUG=chromium:537382 R=adamk,cira LOG=Y ========== to ========== Set the Gregorian changeover date to the beginning of time in Intl ECMAScript dates act as if the Gregorian changeover happened at the beginning of time. This patch fixes up internationalized date formatting to set that changeover properly, as opposed to the ICU default which is in the 16th century. BUG=chromium:537382 R=adamk,cira LOG=Y Committed: https://crrev.com/d67756a7753e322fdd986399677a45a0459a5d40 Cr-Commit-Position: refs/heads/master@{#32669} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/d67756a7753e322fdd986399677a45a0459a5d40 Cr-Commit-Position: refs/heads/master@{#32669} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
