Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2857003002:
[heap] Fix live object iterator bail out case. (Closed)
|
Created:
3 years, 7 months ago by Hannes Payer (out of office) Modified:
3 years, 7 months ago Reviewers:
Michael Lippautz CC:
v8-reviews_googlegroups.com Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
Description[heap] Fix live object iterator bail out case.
BUG=chromium:714207
Review-Url: https://codereview.chromium.org/2857003002
Cr-Commit-Position: refs/heads/master@{#45055}
Committed: https://chromium.googlesource.com/v8/v8/+/f82a59ac30565dd0c4653a22ec7d96d14d050bf2
Patch Set 1 #
Total comments: 1
Patch Set 2 : change checks #Messages
Total messages: 18 (12 generated)
The CQ bit was checked by hpayer@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.
mlippautz@chromium.org changed reviewers: + mlippautz@chromium.org
https://codereview.chromium.org/2857003002/diff/1/src/heap/mark-compact-inl.h File src/heap/mark-compact-inl.h (right): https://codereview.chromium.org/2857003002/diff/1/src/heap/mark-compact-inl.h... src/heap/mark-compact-inl.h:176: // that case we can return immediately. I have a troubles following the iterator and the statement above. Is it possible that what we actually want is DCHECK(!it.Done()); if (!it.Advance()) { ... return nullptr; } // Access the iterator it.Done() checks the current iterator position, right? So in this branch here we should have a valid current position but we are actually not sure whether we can advance? What am I missing?
On 2017/05/03 07:41:35, Michael Lippautz wrote: > https://codereview.chromium.org/2857003002/diff/1/src/heap/mark-compact-inl.h > File src/heap/mark-compact-inl.h (right): > > https://codereview.chromium.org/2857003002/diff/1/src/heap/mark-compact-inl.h... > src/heap/mark-compact-inl.h:176: // that case we can return immediately. > I have a troubles following the iterator and the statement above. Is it possible > that what we actually want is > > DCHECK(!it.Done()); > if (!it.Advance()) { ... return nullptr; } > // Access the iterator > > it.Done() checks the current iterator position, right? So in this branch here we > should have a valid current position but we are actually not sure whether we can > advance? > > What am I missing? You are right, with the change of the Advance semantics the iterator changed. I think Done should only be used in the first while loop and Advance should trigger the bailout. The Advance case in the middle which is not bailing out is guaranteed to succeed (we should DCHECK that).
The CQ bit was checked by hpayer@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.
lgtm *duck*
The CQ bit was checked by hpayer@chromium.org
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": 20001, "attempt_start_ts": 1493806207309490,
"parent_rev": "a525d7c1643c76e18818cabcd9a6b2195505714d", "commit_rev":
"f82a59ac30565dd0c4653a22ec7d96d14d050bf2"}
Message was sent while issue was closed.
Description was changed from ========== [heap] Fix live object iterator bail out case. BUG=chromium:714207 ========== to ========== [heap] Fix live object iterator bail out case. BUG=chromium:714207 Review-Url: https://codereview.chromium.org/2857003002 Cr-Commit-Position: refs/heads/master@{#45055} Committed: https://chromium.googlesource.com/v8/v8/+/f82a59ac30565dd0c4653a22ec7d96d14d0... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/v8/v8/+/f82a59ac30565dd0c4653a22ec7d96d14d0... |
