Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2220293003:
Remove two more special cases from Scope::MustAllocate(Variable*) (Closed)
DescriptionRemove two more special cases from Scope::MustAllocate(Variable*)
Block scopes don't need any special treatment here (it's unclear
to me why they ever did). And the has_forced_context_allocation() check
seems, according to our tests, to only have been necessary for proper
handling of 'with' scopes. This patch instead uses the "is_used" bit
to keep track of variables that are accessed from within a with.
R=neis@chromium.org
Committed: https://crrev.com/2028c0931ecb49ff2625710cade681681dd7ec1c
Cr-Commit-Position: refs/heads/master@{#38505}
Patch Set 1 #Patch Set 2 : Move ForceContextAllocation call down into same block #
Total comments: 2
Patch Set 3 : Add DCHECK #Patch Set 4 : Rebased #Messages
Total messages: 22 (14 generated)
The CQ bit was checked by adamk@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 adamk@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 with comment https://codereview.chromium.org/2220293003/diff/20001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2220293003/diff/20001/src/ast/scopes.cc#newco... src/ast/scopes.cc:1502: return !var->IsGlobalObjectProperty() && var->is_used(); Can you add DCHECK(!var->has_forced_context_allocation() || var->set_is_used()) here?
Description was changed from ========== Remove two more special cases from Scope::MustAllocate(Variable*) Block scopes don't need any special treatment here (it's unclear to me why they ever did). And the has_forced_context_allocation() check seems, according to our tests, to only have been necessary for proper handling of 'with' scopes. This patch instead uses the "is_used" bit to keep track of variables that are accessed from within a with. R=neis@chromium.org ========== to ========== Remove two more special cases from Scope::MustAllocate(Variable*) Block scopes don't need any special treatment here (it's unclear to me why they ever did). And the has_forced_context_allocation() check seems, according to our tests, to only have been necessary for proper handling of 'with' scopes. This patch instead uses the "is_used" bit to keep track of variables that are accessed from within a with. R=neis@chromium.org ==========
https://codereview.chromium.org/2220293003/diff/20001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2220293003/diff/20001/src/ast/scopes.cc#newco... src/ast/scopes.cc:1502: return !var->IsGlobalObjectProperty() && var->is_used(); On 2016/08/09 07:27:15, neis wrote: > Can you add DCHECK(!var->has_forced_context_allocation() || var->set_is_used()) > here? Done.
The CQ bit was checked by adamk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from neis@chromium.org Link to the patchset: https://codereview.chromium.org/2220293003/#ps40001 (title: "Add DCHECK")
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_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/12180) v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)
The CQ bit was checked by adamk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from neis@chromium.org Link to the patchset: https://codereview.chromium.org/2220293003/#ps60001 (title: "Rebased")
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.
Description was changed from ========== Remove two more special cases from Scope::MustAllocate(Variable*) Block scopes don't need any special treatment here (it's unclear to me why they ever did). And the has_forced_context_allocation() check seems, according to our tests, to only have been necessary for proper handling of 'with' scopes. This patch instead uses the "is_used" bit to keep track of variables that are accessed from within a with. R=neis@chromium.org ========== to ========== Remove two more special cases from Scope::MustAllocate(Variable*) Block scopes don't need any special treatment here (it's unclear to me why they ever did). And the has_forced_context_allocation() check seems, according to our tests, to only have been necessary for proper handling of 'with' scopes. This patch instead uses the "is_used" bit to keep track of variables that are accessed from within a with. R=neis@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Remove two more special cases from Scope::MustAllocate(Variable*) Block scopes don't need any special treatment here (it's unclear to me why they ever did). And the has_forced_context_allocation() check seems, according to our tests, to only have been necessary for proper handling of 'with' scopes. This patch instead uses the "is_used" bit to keep track of variables that are accessed from within a with. R=neis@chromium.org ========== to ========== Remove two more special cases from Scope::MustAllocate(Variable*) Block scopes don't need any special treatment here (it's unclear to me why they ever did). And the has_forced_context_allocation() check seems, according to our tests, to only have been necessary for proper handling of 'with' scopes. This patch instead uses the "is_used" bit to keep track of variables that are accessed from within a with. R=neis@chromium.org Committed: https://crrev.com/2028c0931ecb49ff2625710cade681681dd7ec1c Cr-Commit-Position: refs/heads/master@{#38505} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2028c0931ecb49ff2625710cade681681dd7ec1c Cr-Commit-Position: refs/heads/master@{#38505} |
