Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2865113005:
[runtime function] Fix IndexOf when start is -Infinity (Closed)
|
Created:
3 years, 7 months ago by predrag.rudic Modified:
3 years, 7 months ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
Description[runtime function] Fix IndexOf when start is -Infinity
By C++ standard, behaviour is undedined when converting -Infinity to
integer. This patch assures correct behaviour in this case on all
platforms.
BUG=
Review-Url: https://codereview.chromium.org/2865113005
Cr-Commit-Position: refs/heads/master@{#45220}
Committed: https://chromium.googlesource.com/v8/v8/+/a1f00971a183e8a803302ff29065774249d8a132
Patch Set 1 #
Total comments: 4
Patch Set 2 : Corrected #
Total comments: 2
Patch Set 3 : Corrected2 #Messages
Total messages: 19 (11 generated)
Description was changed from ========== [runtime function] Fix IndexOf when start is -Infinity By C++ standard, behaviour is undedined when converting -Infinity to integer. This patch assures correct behaviour in this case on all platforms. BUG= ========== to ========== [runtime function] Fix IndexOf when start is -Infinity By C++ standard, behaviour is undedined when converting -Infinity to integer. This patch assures correct behaviour in this case on all platforms. BUG= ==========
predrag.rudic@imgtec.com changed reviewers: + cbruni@chromium.org, jkummerow@chromium.org, mattl@google.com
predrag.rudic@imgtec.com changed reviewers: + mattloring@google.com - mattl@google.com
PTAL
https://codereview.chromium.org/2865113005/diff/1/src/runtime/runtime-array.cc File src/runtime/runtime-array.cc (right): https://codereview.chromium.org/2865113005/diff/1/src/runtime/runtime-array.c... src/runtime/runtime-array.cc:573: fp < static_cast<double>(std::numeric_limits<int64_t>::max()))) { You don't need the second condition; the "fp > len" check above guarantees that fp <= std::numeric_limits<int64_t>::max(). https://codereview.chromium.org/2865113005/diff/1/src/runtime/runtime-array.c... src/runtime/runtime-array.cc:576: start_from = fp < static_cast<double>(std::numeric_limits<int64_t>::min()) ...and then you don't need a case distinction here either; always assigning std::numeric_limits<int64_t>::min() is fine.
PTAL https://codereview.chromium.org/2865113005/diff/1/src/runtime/runtime-array.cc File src/runtime/runtime-array.cc (right): https://codereview.chromium.org/2865113005/diff/1/src/runtime/runtime-array.c... src/runtime/runtime-array.cc:573: fp < static_cast<double>(std::numeric_limits<int64_t>::max()))) { On 2017/05/09 11:03:32, Jakob Kummerow wrote: > You don't need the second condition; the "fp > len" check above guarantees that > fp <= std::numeric_limits<int64_t>::max(). Acknowledged. https://codereview.chromium.org/2865113005/diff/1/src/runtime/runtime-array.c... src/runtime/runtime-array.cc:576: start_from = fp < static_cast<double>(std::numeric_limits<int64_t>::min()) On 2017/05/09 11:03:32, Jakob Kummerow wrote: > ...and then you don't need a case distinction here either; always assigning > std::numeric_limits<int64_t>::min() is fine. Acknowledged.
LGTM https://codereview.chromium.org/2865113005/diff/20001/src/runtime/runtime-arr... File src/runtime/runtime-array.cc (right): https://codereview.chromium.org/2865113005/diff/20001/src/runtime/runtime-arr... src/runtime/runtime-array.cc:576: start_from = fp = std::numeric_limits<int64_t>::min(); nit: assigning fp is unnecessary.
https://codereview.chromium.org/2865113005/diff/20001/src/runtime/runtime-arr... File src/runtime/runtime-array.cc (right): https://codereview.chromium.org/2865113005/diff/20001/src/runtime/runtime-arr... src/runtime/runtime-array.cc:576: start_from = fp = std::numeric_limits<int64_t>::min(); On 2017/05/09 15:04:23, Jakob Kummerow wrote: > nit: assigning fp is unnecessary. Acknowledged.
lgtm
The CQ bit was checked by predrag.rudic@imgtec.com 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 predrag.rudic@imgtec.com
The patchset sent to the CQ was uploaded after l-g-t-m from jkummerow@chromium.org Link to the patchset: https://codereview.chromium.org/2865113005/#ps40001 (title: "Corrected2")
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": 40001, "attempt_start_ts": 1494405111009730,
"parent_rev": "9fbfd6ead6f7406ca4912610c501146622fa98ea", "commit_rev":
"a1f00971a183e8a803302ff29065774249d8a132"}
Message was sent while issue was closed.
Description was changed from ========== [runtime function] Fix IndexOf when start is -Infinity By C++ standard, behaviour is undedined when converting -Infinity to integer. This patch assures correct behaviour in this case on all platforms. BUG= ========== to ========== [runtime function] Fix IndexOf when start is -Infinity By C++ standard, behaviour is undedined when converting -Infinity to integer. This patch assures correct behaviour in this case on all platforms. BUG= Review-Url: https://codereview.chromium.org/2865113005 Cr-Commit-Position: refs/heads/master@{#45220} Committed: https://chromium.googlesource.com/v8/v8/+/a1f00971a183e8a803302ff29065774249d... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/a1f00971a183e8a803302ff29065774249d... |
