Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2217973003:
Clean up and simplify Parser::Declare (Closed)
DescriptionClean up and simplify Parser::Declare
This removes the "resolve" parameter from Declare, since there's no case
where Scope analysis wouldn't make the same resolution decision the
parser can make here.
Other cleanup includes:
- Replace some if statements with DCHECKS where they could never be hit
- After the above, reordered some conditions for clarity
- Remove or edit down old comments
Should be no change in behavior.
R=neis@chromium.org
Committed: https://crrev.com/9b6c57045d36d2bf53b529dc83efc1b44491ea6f
Cr-Commit-Position: refs/heads/master@{#38454}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebased #Patch Set 3 : Fix bad merge #
Messages
Total messages: 26 (15 generated)
The CQ bit was checked by adamk@chromium.org
The CQ bit was unchecked by adamk@chromium.org
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.
For the record, it looks like the "resolve" argument has been mostly a no-op for just over 5 years: prior to http://codereview.chromium.org/7280012, we would re-use the VariableProxy created as part of calling Declare() (actually returned from it at that time) when desugaring var x = 7; into var x; x = 7; (which is required to get the proper behavior in 'with' and 'catch' contexts). But that CL introduced a new VariableProxy for the assignment, which rendered the delaying of resolution moot.
Patchset #2 (id:20001) has been deleted
Great, lgtm. One comment below. https://codereview.chromium.org/2217973003/diff/1/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2217973003/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:2029: if (is_sloppy(scope->language_mode()) && is_function_declaration && Was this just wrong before?
https://codereview.chromium.org/2217973003/diff/1/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2217973003/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:2029: if (is_sloppy(scope->language_mode()) && is_function_declaration && On 2016/08/08 09:42:34, neis wrote: > Was this just wrong before? Yes, but not in any observable way (due to what callers pass for "scope").
The CQ bit was checked by adamk@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_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/10571) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/10556) 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_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
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/2217973003/#ps40001 (title: "Rebased")
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_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
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/2217973003/#ps60001 (title: "Fix bad merge")
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 #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Clean up and simplify Parser::Declare This removes the "resolve" parameter from Declare, since there's no case where Scope analysis wouldn't make the same resolution decision the parser can make here. Other cleanup includes: - Replace some if statements with DCHECKS where they could never be hit - After the above, reordered some conditions for clarity - Remove or edit down old comments Should be no change in behavior. R=neis@chromium.org ========== to ========== Clean up and simplify Parser::Declare This removes the "resolve" parameter from Declare, since there's no case where Scope analysis wouldn't make the same resolution decision the parser can make here. Other cleanup includes: - Replace some if statements with DCHECKS where they could never be hit - After the above, reordered some conditions for clarity - Remove or edit down old comments Should be no change in behavior. R=neis@chromium.org Committed: https://crrev.com/9b6c57045d36d2bf53b529dc83efc1b44491ea6f Cr-Commit-Position: refs/heads/master@{#38454} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9b6c57045d36d2bf53b529dc83efc1b44491ea6f Cr-Commit-Position: refs/heads/master@{#38454} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
