Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2294913004:
[printing] Fix DCHECK failure when printing FAST_HOLEY_DOUBLE_ELEMENTS (Closed)
|
Created:
4 years, 3 months ago by Camillo Bruni Modified:
4 years, 3 months ago CC:
Hannes Payer (out of office), ulan, v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[printing] Fix DCHECK failure when printing FAST_HOLEY_DOUBLE_ELEMENTS
This CL fixes %DebugPrint for FAST_HOLEY_DOUBLE_ELEMENTS and now properly
distinguishes TheHole and NaN values.
BUG=
Committed: https://crrev.com/cd86053facda29f5ea2edb47c24f4f6d99f1520a
Cr-Commit-Position: refs/heads/master@{#39293}
Patch Set 1 #Patch Set 2 : adding test #
Total comments: 4
Patch Set 3 : addressing nits #Patch Set 4 : rebasing #Patch Set 5 : revert accidental changes #Patch Set 6 : removing global hole double #Messages
Total messages: 26 (12 generated)
cbruni@chromium.org changed reviewers: + jkummerow@chromium.org, ulan@chromium.org
ulan@ PTAL heap.cc jkummerow@ PTAL the rest
LGTM with nits. https://codereview.chromium.org/2294913004/diff/20001/src/objects-printer.cc File src/objects-printer.cc (right): https://codereview.chromium.org/2294913004/diff/20001/src/objects-printer.cc#... src/objects-printer.cc:351: // Values are not equal if only one of the is the hole. nit: s/the/them/ https://codereview.chromium.org/2294913004/diff/20001/src/objects-printer.cc#... src/objects-printer.cc:352: if (!(is_the_hole(previous_value) ^ is_the_hole(value))) { nit: !(a ^ b) === (a == b) ...and after that replacement I'd also fold it into the previous condition. https://codereview.chromium.org/2294913004/diff/20001/test/mjsunit/debug-prin... File test/mjsunit/debug-print.js (right): https://codereview.chromium.org/2294913004/diff/20001/test/mjsunit/debug-prin... test/mjsunit/debug-print.js:7: // Make sure printing different kinds of objects doesn't crash nit: trailing full stop please. https://codereview.chromium.org/2294913004/diff/20001/test/mjsunit/debug-prin... test/mjsunit/debug-print.js:9: // Different elements Did you mean to apply s/objects/elements/ to the previous line?
heap lgtm
The CQ bit was checked by cbruni@chromium.org
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/2294913004/#ps40001 (title: "addressing nits")
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_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/8046) v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/23081)
The CQ bit was checked by cbruni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org, jkummerow@chromium.org Link to the patchset: https://codereview.chromium.org/2294913004/#ps80001 (title: "revert accidental changes")
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_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/11989) v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
The CQ bit was checked by cbruni@chromium.org
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_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/11997) v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
It seems you can't use bit_cast to initialize a global const double... too bad. What's the plan, wanna fix and land this CL?
On 2016/09/07 at 13:09:11, jkummerow wrote: > It seems you can't use bit_cast to initialize a global const double... too bad. What's the plan, wanna fix and land this CL? Going to refactor and land it... spreading the bit_cast back to all the users.
The CQ bit was checked by cbruni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org, jkummerow@chromium.org Link to the patchset: https://codereview.chromium.org/2294913004/#ps100001 (title: "removing global hole double")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [printing] Fix DCHECK failure when printing FAST_HOLEY_DOUBLE_ELEMENTS This CL fixes %DebugPrint for FAST_HOLEY_DOUBLE_ELEMENTS and now properly distinguishes TheHole and NaN values. BUG= ========== to ========== [printing] Fix DCHECK failure when printing FAST_HOLEY_DOUBLE_ELEMENTS This CL fixes %DebugPrint for FAST_HOLEY_DOUBLE_ELEMENTS and now properly distinguishes TheHole and NaN values. BUG= Committed: https://crrev.com/cd86053facda29f5ea2edb47c24f4f6d99f1520a Cr-Commit-Position: refs/heads/master@{#39293} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/cd86053facda29f5ea2edb47c24f4f6d99f1520a Cr-Commit-Position: refs/heads/master@{#39293} |
