Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2520363002:
Reimplement Number.prototype.toString with non-default radix. (Closed)
DescriptionReimplement Number.prototype.toString with non-default radix.
The old algorithm produces unnecessary decimal digits. The new one
converts the significand of the input double into an uint64_t to be
just as precise as necessary.
R=tebbi@chromium.org
BUG=chromium:658712, chromium:666376
Committed: https://crrev.com/21b0dbedfd470b67c47f84428e1af95a506e49e5
Cr-Commit-Position: refs/heads/master@{#41255}
Patch Set 1 #
Total comments: 2
Patch Set 2 : . #Patch Set 3 : reimplemented #Patch Set 4 : readd copyright headers #
Total comments: 2
Messages
Total messages: 32 (19 generated)
littledan@chromium.org changed reviewers: + littledan@chromium.org
lgtm Very clean implementation. Do you want to mention the bug 666376 as well? This seems to fix that too.
lgtm https://codereview.chromium.org/2520363002/diff/1/src/builtins/builtins-numbe... File src/builtins/builtins-number.cc (right): https://codereview.chromium.org/2520363002/diff/1/src/builtins/builtins-numbe... src/builtins/builtins-number.cc:549: value_number == 0.0) { nit: value_number == -0.0 https://codereview.chromium.org/2520363002/diff/1/src/conversions.cc File src/conversions.cc (right): https://codereview.chromium.org/2520363002/diff/1/src/conversions.cc#newcode450 src/conversions.cc:450: uint64_t int_value = static_cast<uint64_t>(value); (2/7).toString(3) gives "0.0212010212010212010212010212010211" instead of "0.0212010212010212010212010212010212" This is not just the wrong decision weather to round up or down, the last digit is just wrong. To fix this, we would need to restrict ourseleves to less significant digits and then do proper rounding on the int value.
On 2016/11/22 18:46:54, Tobias Tebbi wrote: > lgtm > > https://codereview.chromium.org/2520363002/diff/1/src/builtins/builtins-numbe... > File src/builtins/builtins-number.cc (right): > > https://codereview.chromium.org/2520363002/diff/1/src/builtins/builtins-numbe... > src/builtins/builtins-number.cc:549: value_number == 0.0) { > nit: value_number == -0.0 > > https://codereview.chromium.org/2520363002/diff/1/src/conversions.cc > File src/conversions.cc (right): > > https://codereview.chromium.org/2520363002/diff/1/src/conversions.cc#newcode450 > src/conversions.cc:450: uint64_t int_value = static_cast<uint64_t>(value); > (2/7).toString(3) gives "0.0212010212010212010212010212010211" instead of > "0.0212010212010212010212010212010212" > This is not just the wrong decision weather to round up or down, the last digit > is just wrong. To fix this, we would need to restrict ourseleves to less > significant digits and then do proper rounding on the int value. I dont think the double representation of 2/7 is precise enough for this.
The CQ bit was checked by yangguo@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...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
The CQ bit was checked by yangguo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tebbi@chromium.org, littledan@chromium.org Link to the patchset: https://codereview.chromium.org/2520363002/#ps20001 (title: ".")
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_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/13042) v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)
The CQ bit was checked by yangguo@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...
Reimplemented this due to imprecisions. This is also faster than the two previous variants.
Description was changed from ========== Reimplement Number.prototype.toString with non-default radix. The old algorithm produces unnecessary decimal digits. The new one converts the significand of the input double into an uint64_t to be just as precise as necessary. R=tebbi@chromium.org BUG=chromium:658712 ========== to ========== Reimplement Number.prototype.toString with non-default radix. The old algorithm produces unnecessary decimal digits. The new one converts the significand of the input double into an uint64_t to be just as precise as necessary. R=tebbi@chromium.org BUG=chromium:658712,chromium:666376 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by yangguo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from littledan@chromium.org Link to the patchset: https://codereview.chromium.org/2520363002/#ps60001 (title: "readd copyright headers")
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": 60001, "attempt_start_ts": 1479983299040780,
"parent_rev": "8993b59043e8c19a5eb79e6419e6b90522df631c", "commit_rev":
"faa1fa8bbd34603f27064b03d01ae13f0e33b9ee"}
Message was sent while issue was closed.
Description was changed from ========== Reimplement Number.prototype.toString with non-default radix. The old algorithm produces unnecessary decimal digits. The new one converts the significand of the input double into an uint64_t to be just as precise as necessary. R=tebbi@chromium.org BUG=chromium:658712,chromium:666376 ========== to ========== Reimplement Number.prototype.toString with non-default radix. The old algorithm produces unnecessary decimal digits. The new one converts the significand of the input double into an uint64_t to be just as precise as necessary. R=tebbi@chromium.org BUG=chromium:658712,chromium:666376 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Reimplement Number.prototype.toString with non-default radix. The old algorithm produces unnecessary decimal digits. The new one converts the significand of the input double into an uint64_t to be just as precise as necessary. R=tebbi@chromium.org BUG=chromium:658712,chromium:666376 ========== to ========== Reimplement Number.prototype.toString with non-default radix. The old algorithm produces unnecessary decimal digits. The new one converts the significand of the input double into an uint64_t to be just as precise as necessary. R=tebbi@chromium.org BUG=chromium:658712,chromium:666376 Committed: https://crrev.com/21b0dbedfd470b67c47f84428e1af95a506e49e5 Cr-Commit-Position: refs/heads/master@{#41255} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/21b0dbedfd470b67c47f84428e1af95a506e49e5 Cr-Commit-Position: refs/heads/master@{#41255}
Message was sent while issue was closed.
jkummerow@chromium.org changed reviewers: + jkummerow@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2520363002/diff/60001/src/builtins/builtins-n... File src/builtins/builtins-number.cc (right): https://codereview.chromium.org/2520363002/diff/60001/src/builtins/builtins-n... src/builtins/builtins-number.cc:549: value_number == -0.0) { DBC: This is misleading, because "value_number == -0.0" also evaluates to true when value_number == 0.0. That's fine here, but could lead to bugs when this pattern is copied elsewhere. Use IsMinusZero() from conversions.h to actually check for -0, or compare against 0.0 with a comment like "Covers both 0 and -0." to make explicit what's going on.
Message was sent while issue was closed.
https://codereview.chromium.org/2520363002/diff/60001/src/builtins/builtins-n... File src/builtins/builtins-number.cc (right): https://codereview.chromium.org/2520363002/diff/60001/src/builtins/builtins-n... src/builtins/builtins-number.cc:549: value_number == -0.0) { On 2016/11/28 12:15:11, Jakob Kummerow wrote: > DBC: This is misleading, because "value_number == -0.0" also evaluates to true > when value_number == 0.0. That's fine here, but could lead to bugs when this > pattern is copied elsewhere. > > Use IsMinusZero() from conversions.h to actually check for -0, or compare > against 0.0 with a comment like "Covers both 0 and -0." to make explicit what's > going on. Noted. I originally had compared to 0.0 in patch set 1. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
