Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2573283003:
Refactor IterationHasObservableEffects to be more readable. (Closed)
|
Created:
4 years ago by petermarshall Modified:
4 years ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionRefactor IterationHasObservableEffects to be more readable.
This was kind of hard to read due to the nesting. Refactor it using short-circuit
a bit more and add some comments to each bit.
Review-Url: https://codereview.chromium.org/2573283003
Cr-Commit-Position: refs/heads/master@{#41727}
Committed: https://chromium.googlesource.com/v8/v8/+/c911517f0d763be4c556ebe8743382e363a0310d
Patch Set 1 #
Total comments: 1
Patch Set 2 : Get rid of silly else #
Total comments: 1
Patch Set 3 : remove potentially confusing todo #Messages
Total messages: 22 (13 generated)
Description was changed from ========== Refactor IterationHasObservableEffects to be more readable. ========== to ========== Refactor IterationHasObservableEffects to be more readable. This was kind of hard to read due to the nesting. Refactor it using short-circuit a bit more. ==========
Description was changed from ========== Refactor IterationHasObservableEffects to be more readable. This was kind of hard to read due to the nesting. Refactor it using short-circuit a bit more. ========== to ========== Refactor IterationHasObservableEffects to be more readable. This was kind of hard to read due to the nesting. Refactor it using short-circuit a bit more and add some comments to each bit. ==========
petermarshall@chromium.org changed reviewers: + franzih@chromium.org
PTAL and let me know if you think this is any better than before =]
The CQ bit was checked by petermarshall@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...
I think that's easier to read. LGTM with nit. https://codereview.chromium.org/2573283003/diff/1/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2573283003/diff/1/src/objects.cc#newcode2214 src/objects.cc:2214: } else { If you return inside an if, you don't need an else. if () { ... return false; } return true;
The CQ bit was checked by petermarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from franzih@chromium.org Link to the patchset: https://codereview.chromium.org/2573283003/#ps20001 (title: "Get rid of silly else")
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_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/30681)
petermarshall@chromium.org changed reviewers: + mstarzinger@chromium.org
mstarzinger could you please take a look as an owner =]
LGTM. https://codereview.chromium.org/2573283003/diff/20001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2573283003/diff/20001/src/objects.cc#newcode2195 src/objects.cc:2195: // changes that a map check won't recognize. question: Not sure about this TODO, which changes to the iterator do you have in mind? If we are not sure what the concrete issue here is we could file an issue on the issue tracker instead.
On 2016/12/15 at 14:49:44, petermarshall wrote: > mstarzinger could you please take a look as an owner =] Removed the TODO because we aren't 100% sure if there is a problem there.
The CQ bit was checked by petermarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from franzih@chromium.org Link to the patchset: https://codereview.chromium.org/2573283003/#ps40001 (title: "remove potentially confusing todo")
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": 1481814389904630,
"parent_rev": "45d51734c973f517335f1240b9a85dd175d5288b", "commit_rev":
"c911517f0d763be4c556ebe8743382e363a0310d"}
Message was sent while issue was closed.
Description was changed from ========== Refactor IterationHasObservableEffects to be more readable. This was kind of hard to read due to the nesting. Refactor it using short-circuit a bit more and add some comments to each bit. ========== to ========== Refactor IterationHasObservableEffects to be more readable. This was kind of hard to read due to the nesting. Refactor it using short-circuit a bit more and add some comments to each bit. Review-Url: https://codereview.chromium.org/2573283003 Cr-Commit-Position: refs/heads/master@{#41727} Committed: https://chromium.googlesource.com/v8/v8/+/c911517f0d763be4c556ebe8743382e363a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/c911517f0d763be4c556ebe8743382e363a... |
