Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2839623003:
[modules] Factor out cell load into helper function. (Closed)
|
Created:
3 years, 8 months ago by neis Modified:
3 years, 7 months ago Reviewers:
Michael Starzinger CC:
v8-reviews_googlegroups.com Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
Description[modules] Factor out cell load into helper function.
This is just a refactoring, no changes in behavior.
BUG=v8:1569
Review-Url: https://codereview.chromium.org/2839623003
Cr-Commit-Position: refs/heads/master@{#45071}
Committed: https://chromium.googlesource.com/v8/v8/+/973ec26eb174ad434a04836b93aa739e320ae89d
Patch Set 1 #
Total comments: 3
Patch Set 2 : Make GetCell a method. #
Total comments: 1
Patch Set 3 : Assign cell in unreachable path. #
Messages
Total messages: 30 (22 generated)
The CQ bit was checked by neis@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 checked by neis@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...
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by neis@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.
The CQ bit was checked by neis@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...
Patchset #1 (id:60001) has been deleted
Description was changed from ========== [modules] Factor out cell load into helper function. This is just a refactoring, no changes in behavior. BUG=v8:1569 ========== to ========== [modules] Factor out cell load into helper function. This is just a refactoring, no changes in behavior. BUG=v8:1569 ==========
neis@chromium.org changed reviewers: + mstarzinger@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with comments. https://codereview.chromium.org/2839623003/diff/80001/src/compiler/js-typed-l... File src/compiler/js-typed-lowering.cc (right): https://codereview.chromium.org/2839623003/diff/80001/src/compiler/js-typed-l... src/compiler/js-typed-lowering.cc:1428: if (cell->op()->EffectOutputCount() > 0) effect = cell; The returned {LoadField} node will always have an effect output. Can we just unconditionally say "Node* cell = effect = BuildGetModuleCell(node)" here? If we want to play it safe we could turn this into a DCHECK. https://codereview.chromium.org/2839623003/diff/80001/src/compiler/js-typed-l... src/compiler/js-typed-lowering.cc:1446: if (cell->op()->EffectOutputCount() > 0) effect = cell; Likewise. https://codereview.chromium.org/2839623003/diff/80001/src/objects.h File src/objects.h (right): https://codereview.chromium.org/2839623003/diff/80001/src/objects.h#newcode6736 src/objects.h:6736: static Cell* GetCell(Module* module, int cell_index); nit: Since this is an unhandlified method, I would vote for it being a true member method instead of a static helper. I am thinking along the lines of "Cell* GetCall(int cell_index)" here. Also it seems this is an internal helper. Would it make sense to make it private?
On 2017/04/27 09:51:02, Michael Starzinger wrote: > LGTM with comments. > > https://codereview.chromium.org/2839623003/diff/80001/src/compiler/js-typed-l... > File src/compiler/js-typed-lowering.cc (right): > > https://codereview.chromium.org/2839623003/diff/80001/src/compiler/js-typed-l... > src/compiler/js-typed-lowering.cc:1428: if (cell->op()->EffectOutputCount() > 0) > effect = cell; > The returned {LoadField} node will always have an effect output. Can we just > unconditionally say "Node* cell = effect = BuildGetModuleCell(node)" here? If we > want to play it safe we could turn this into a DCHECK. > > https://codereview.chromium.org/2839623003/diff/80001/src/compiler/js-typed-l... > src/compiler/js-typed-lowering.cc:1446: if (cell->op()->EffectOutputCount() > 0) > effect = cell; > Likewise. > > https://codereview.chromium.org/2839623003/diff/80001/src/objects.h > File src/objects.h (right): > > https://codereview.chromium.org/2839623003/diff/80001/src/objects.h#newcode6736 > src/objects.h:6736: static Cell* GetCell(Module* module, int cell_index); > nit: Since this is an unhandlified method, I would vote for it being a true > member method instead of a static helper. I am thinking along the lines of > "Cell* GetCall(int cell_index)" here. > > Also it seems this is an internal helper. Would it make sense to make it > private? Hi Michi, I should have mentioned it in the CL description: the dependent CL may make it clear why I did these things. Regarding the first point, in the dependent CL the result is not always a LoadField but sometimes a constant. Regarding GetCell, in the dependent CL it's used elsewhere so it can't be private. I agree, though, that it shouldn't be static.
On 2017/04/27 09:57:54, neis wrote: > I agree, though, that it shouldn't be static. Changed now.
Michael, can I land this in its current form? Please also have a look at the followup CL.
LGTM on patch set #2. Fine with landing as is. https://codereview.chromium.org/2839623003/diff/100001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2839623003/diff/100001/src/objects.cc#newcode... src/objects.cc:20152: Object* cell; nit: I have the sneaking suspicion that some compilers will complain about the "cell" variable not being initialized on all paths in some configurations.
The CQ bit was checked by neis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstarzinger@chromium.org Link to the patchset: https://codereview.chromium.org/2839623003/#ps120001 (title: "Assign cell in unreachable path.")
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": 120001, "attempt_start_ts": 1493832613012740,
"parent_rev": "ce5ffd931cad057e551de25f3a42b7b358e146ec", "commit_rev":
"973ec26eb174ad434a04836b93aa739e320ae89d"}
Message was sent while issue was closed.
Description was changed from ========== [modules] Factor out cell load into helper function. This is just a refactoring, no changes in behavior. BUG=v8:1569 ========== to ========== [modules] Factor out cell load into helper function. This is just a refactoring, no changes in behavior. BUG=v8:1569 Review-Url: https://codereview.chromium.org/2839623003 Cr-Commit-Position: refs/heads/master@{#45071} Committed: https://chromium.googlesource.com/v8/v8/+/973ec26eb174ad434a04836b93aa739e320... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:120001) as https://chromium.googlesource.com/v8/v8/+/973ec26eb174ad434a04836b93aa739e320... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
