Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1845223006:
[Interpreter] Handles legacy constants in strict mode. (Closed)
Description[Interpreter] Handles legacy constants in strict mode.
Function bindings are the only variables in LEGACY_CONST mode.
(https://codereview.chromium.org/1819123002/). Since these variables
can also be accessed in strict mode functions we should support
handling such variables. Assigning to a legacy constant throws
a TypeError in strict mode. Also fixes hydrogen.cc to throw a
TypeError for legacy constants.
BUG=v8:4280, chromium:599068
LOG=N
TBR=rmcilroy@chromium.org
Committed: https://crrev.com/8982cb5c70c8371256c6e9183d3ef3b33e46f056
Cr-Commit-Position: refs/heads/master@{#35383}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Fixes the implementation and modifies the test. #Patch Set 3 : Fixed compound assignments in hydrogen. Updated test as suggested by mstarzinger@ offline. #
Total comments: 6
Patch Set 4 : Fixed nits and added another test. #Patch Set 5 : Addressed nits. #
Messages
Total messages: 28 (13 generated)
mythria@chromium.org changed reviewers: + mstarzinger@chromium.org
PTAL. Thanks, Mythri
https://codereview.chromium.org/1845223006/diff/1/test/mjsunit/ignition/regre... File test/mjsunit/ignition/regress-599068-func-bindings.js (right): https://codereview.chromium.org/1845223006/diff/1/test/mjsunit/ignition/regre... test/mjsunit/ignition/regress-599068-func-bindings.js:8: encodeURI += 'function'; Maybe I am missing something here, but this should throw a TypeError IIRC. And it does so in full-codegen. Can we adapt the test case to actually call the "Func" function? I was thinking about a test case like the following ... (function f() { function assignSloppy() { f = 0; } assertDoesNotThrow(assignSloppy); function assignStrict() { 'use strict'; f = 0; } assertThrows(assignStrict); })();
The CQ bit was checked by mythria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1845223006/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1845223006/20001
Description was changed from ========== [Interpreter] Removes DCHECK for strict mode when processing legacy constants. Function bindings are the only variables in LEGACY_CONST mode. (https://codereview.chromium.org/1819123002/). Since these variables can also be accessed in strict mode functions we should not check for strict mode when processing legacy constants. BUG=v8:4280,chromium:599068 LOG=N TBR=rmcilroy@chromium.org ========== to ========== [Interpreter] Handles legacy constants in strict mode. Function bindings are the only variables in LEGACY_CONST mode. (https://codereview.chromium.org/1819123002/). Since these variables can also be accessed in strict mode functions we should support handling such variables. Assigning to a legacy constant throws a TypeError in strict mode. Also fixes hydrogen.cc to throw a TypeError for legacy constants. BUG=v8:4280,chromium:599068 LOG=N TBR=rmcilroy@chromium.org ==========
Description was changed from ========== [Interpreter] Handles legacy constants in strict mode. Function bindings are the only variables in LEGACY_CONST mode. (https://codereview.chromium.org/1819123002/). Since these variables can also be accessed in strict mode functions we should support handling such variables. Assigning to a legacy constant throws a TypeError in strict mode. Also fixes hydrogen.cc to throw a TypeError for legacy constants. BUG=v8:4280,chromium:599068 LOG=N TBR=rmcilroy@chromium.org ========== to ========== [Interpreter] Handles legacy constants in strict mode. Function bindings are the only variables in LEGACY_CONST mode. (https://codereview.chromium.org/1819123002/). Since these variables can also be accessed in strict mode functions we should support handling such variables. Assigning to a legacy constant throws a TypeError in strict mode. Also fixes hydrogen.cc to throw a TypeError for legacy constants. BUG=v8:4280,chromium:599068 LOG=N TBR=rmcilroy@chromium.org ==========
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 mythria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1845223006/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1845223006/40001
Thanks Michi for tests and suggestions. I implemented another fix to hydrogen as discussed offline. I also moved the test from mjsunit/ignition to mjsunit/regress because we also test for crankshaft. PTAL. I will be away next week. I can work on this cl on Monday. Tuesday-Friday I am totally off. No laptop :). Thanks, Mythri
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with two nits. https://codereview.chromium.org/1845223006/diff/40001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1845223006/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:2079: if (is_strict(language_mode())) { nit: The Runtime_StoreLookupSlot_[Strict|Sloppy] method family should handle this case correctly. I think we can just remove the entire {mode == CONST_LEGACY && op != Token::INIT} case and rely on the runtime function to do the right thing IIUC. https://codereview.chromium.org/1845223006/diff/40001/test/mjsunit/regress/re... File test/mjsunit/regress/regress-599068-func-bindings.js (right): https://codereview.chromium.org/1845223006/diff/40001/test/mjsunit/regress/re... test/mjsunit/regress/regress-599068-func-bindings.js:8: (function f() { nit: Indentation within this function is off.
LGTM with Michi's comments addressed. https://codereview.chromium.org/1845223006/diff/40001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1845223006/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:2049: // in sloppy mode. Break hear to avoid storing into variable. /s/hear/here
Thanks, I fixed the comments and also added a test for lookup slots. Thanks, Mythri https://codereview.chromium.org/1845223006/diff/40001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1845223006/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:2049: // in sloppy mode. Break hear to avoid storing into variable. On 2016/04/05 08:44:21, rmcilroy wrote: > /s/hear/here Done. https://codereview.chromium.org/1845223006/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:2079: if (is_strict(language_mode())) { On 2016/04/04 12:02:24, Michael Starzinger wrote: > nit: The Runtime_StoreLookupSlot_[Strict|Sloppy] method family should handle > this case correctly. I think we can just remove the entire {mode == CONST_LEGACY > && op != Token::INIT} case and rely on the runtime function to do the right > thing IIUC. Done. https://codereview.chromium.org/1845223006/diff/40001/test/mjsunit/regress/re... File test/mjsunit/regress/regress-599068-func-bindings.js (right): https://codereview.chromium.org/1845223006/diff/40001/test/mjsunit/regress/re... test/mjsunit/regress/regress-599068-func-bindings.js:8: (function f() { On 2016/04/04 12:02:24, Michael Starzinger wrote: > nit: Indentation within this function is off. Done.
The CQ bit was checked by mythria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1845223006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1845223006/80001
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 mythria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org, mstarzinger@chromium.org Link to the patchset: https://codereview.chromium.org/1845223006/#ps80001 (title: "Addressed nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1845223006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1845223006/80001
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Handles legacy constants in strict mode. Function bindings are the only variables in LEGACY_CONST mode. (https://codereview.chromium.org/1819123002/). Since these variables can also be accessed in strict mode functions we should support handling such variables. Assigning to a legacy constant throws a TypeError in strict mode. Also fixes hydrogen.cc to throw a TypeError for legacy constants. BUG=v8:4280,chromium:599068 LOG=N TBR=rmcilroy@chromium.org ========== to ========== [Interpreter] Handles legacy constants in strict mode. Function bindings are the only variables in LEGACY_CONST mode. (https://codereview.chromium.org/1819123002/). Since these variables can also be accessed in strict mode functions we should support handling such variables. Assigning to a legacy constant throws a TypeError in strict mode. Also fixes hydrogen.cc to throw a TypeError for legacy constants. BUG=v8:4280,chromium:599068 LOG=N TBR=rmcilroy@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Handles legacy constants in strict mode. Function bindings are the only variables in LEGACY_CONST mode. (https://codereview.chromium.org/1819123002/). Since these variables can also be accessed in strict mode functions we should support handling such variables. Assigning to a legacy constant throws a TypeError in strict mode. Also fixes hydrogen.cc to throw a TypeError for legacy constants. BUG=v8:4280,chromium:599068 LOG=N TBR=rmcilroy@chromium.org ========== to ========== [Interpreter] Handles legacy constants in strict mode. Function bindings are the only variables in LEGACY_CONST mode. (https://codereview.chromium.org/1819123002/). Since these variables can also be accessed in strict mode functions we should support handling such variables. Assigning to a legacy constant throws a TypeError in strict mode. Also fixes hydrogen.cc to throw a TypeError for legacy constants. BUG=v8:4280,chromium:599068 LOG=N TBR=rmcilroy@chromium.org Committed: https://crrev.com/8982cb5c70c8371256c6e9183d3ef3b33e46f056 Cr-Commit-Position: refs/heads/master@{#35383} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8982cb5c70c8371256c6e9183d3ef3b33e46f056 Cr-Commit-Position: refs/heads/master@{#35383} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
