[cleanup] Remove dead code from DeclareLookupSlot and rename it

[cleanup] Remove dead code from DeclareLookupSlot and rename it

Runtime_DeclareLookupSlot is used when generating code for var and function declarations
originating in an eval. Over time, it's accumulated quite a bit of cruft, which this CL removes:

  - With legacy const gone, lookup slots never have any property attributes.
  - There was a bit signaling that the variable was from an eval, but that was redundant since
    DeclareLookupSlot is only used for eval.
  - Some Proxy-related code didn't make sense here.

Its name was also not terribly clear: while "LookupSlot" is used in several places, this
particular function is only used for declaring variables and functions inside sloppy eval.
Renamed (and split into two) to make this clear for future archeologists.

Also added various DCHECKs to check the assumptions being made.

Committed: https://crrev.com/cbc6adc86cc2697785d311ff5bf9f736c7d9e725
Cr-Commit-Position: refs/heads/master@{#37111}

[adamk, 2016-06-14 09:29:24]

Description was changed from

==========
Simplify DeclareLookupSlot
==========

to

==========
[cleanup] Remove dead code from DeclareLookupSlot

Runtime_DeclareLookupSlot is used when generating code for var and function
declarations
originating in an eval. Over time, it's accumulated quite a bit of cruft, which
this CL removes:

  - With legacy const gone, lookup slots never have any property attributes.
  - There was a bit signaling that the variable was from an eval, but that was
redundant since
    DeclareLookupSlot is only used for eval.
  - Some Proxy-related code didn't make sense here.

Also added various DCHECKs to check the assumptions being made.
==========

[adamk, 2016-06-14 09:29:50]

adamk@chromium.org changed reviewers:
+ mstarzinger@chromium.org, verwaest@chromium.org

[Michael Starzinger, 2016-06-14 11:11:37]

LGTM with comments.

https://codereview.chromium.org/2061173002/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2061173002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:763:
register_allocator()->PrepareForConsecutiveAllocations(3);
nit: s/3/2/ here, only two consecutive register needed now.

https://codereview.chromium.org/2061173002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:807:
register_allocator()->PrepareForConsecutiveAllocations(3);
nit: Likewise s/3/2/ here.

[adamk, 2016-06-14 11:23:07]

Thanks for the review. Toon, do you want to take a look before I land?

https://codereview.chromium.org/2061173002/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2061173002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:763:
register_allocator()->PrepareForConsecutiveAllocations(3);
On 2016/06/14 11:11:37, Michael Starzinger wrote:
> nit: s/3/2/ here, only two consecutive register needed now.

Done.

https://codereview.chromium.org/2061173002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:807:
register_allocator()->PrepareForConsecutiveAllocations(3);
On 2016/06/14 11:11:37, Michael Starzinger wrote:
> nit: Likewise s/3/2/ here.

Done.

[adamk, 2016-06-14 14:42:18]

Description was changed from

==========
[cleanup] Remove dead code from DeclareLookupSlot

Runtime_DeclareLookupSlot is used when generating code for var and function
declarations
originating in an eval. Over time, it's accumulated quite a bit of cruft, which
this CL removes:

  - With legacy const gone, lookup slots never have any property attributes.
  - There was a bit signaling that the variable was from an eval, but that was
redundant since
    DeclareLookupSlot is only used for eval.
  - Some Proxy-related code didn't make sense here.

Also added various DCHECKs to check the assumptions being made.
==========

to

==========
[cleanup] Remove dead code from DeclareLookupSlot and rename it

Runtime_DeclareLookupSlot is used when generating code for var and function
declarations
originating in an eval. Over time, it's accumulated quite a bit of cruft, which
this CL removes:

  - With legacy const gone, lookup slots never have any property attributes.
  - There was a bit signaling that the variable was from an eval, but that was
redundant since
    DeclareLookupSlot is only used for eval.
  - Some Proxy-related code didn't make sense here.

Its name was also not terribly clear: while "LookupSlot" is used in several
places, this
particular function is only used for declaring variables and functions inside
sloppy eval.
Renamed (and split into two) to make this clear for future archeologists.

Also added various DCHECKs to check the assumptions being made.
==========

[adamk, 2016-06-14 14:43:05]

As discussed with Toon offline, I've done some more work to clarify the use of
this runtime function. PTAL.

[Michael Starzinger, 2016-06-14 14:47:32]

LGTM with nits. I like the two new runtime functions. Good idea.

https://codereview.chromium.org/2061173002/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2061173002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:763:
register_allocator()->PrepareForConsecutiveAllocations(1);
nit: For a single register, there is no need to prepare for consecutive
registers. This line can be dropped now.

https://codereview.chromium.org/2061173002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:764: Register name =
register_allocator()->NextConsecutiveRegister();
nit: This can be just register_allocator()->NewRegister() now.

https://codereview.chromium.org/2061173002/diff/80001/src/runtime/runtime-sco...
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/2061173002/diff/80001/src/runtime/runtime-sco...
src/runtime/runtime-scopes.cc:191: static Object* DeclareEvalHelper(Isolate*
isolate, Handle<String> name,
nit: Please use an anonymous namespace again instead of a static function.

[adamk, 2016-06-14 14:59:43]

https://codereview.chromium.org/2061173002/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2061173002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:763:
register_allocator()->PrepareForConsecutiveAllocations(1);
On 2016/06/14 14:47:32, Michael Starzinger wrote:
> nit: For a single register, there is no need to prepare for consecutive
> registers. This line can be dropped now.

Done.

https://codereview.chromium.org/2061173002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:764: Register name =
register_allocator()->NextConsecutiveRegister();
On 2016/06/14 14:47:32, Michael Starzinger wrote:
> nit: This can be just register_allocator()->NewRegister() now.

Done.

https://codereview.chromium.org/2061173002/diff/80001/src/runtime/runtime-sco...
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/2061173002/diff/80001/src/runtime/runtime-sco...
src/runtime/runtime-scopes.cc:191: static Object* DeclareEvalHelper(Isolate*
isolate, Handle<String> name,
On 2016/06/14 14:47:32, Michael Starzinger wrote:
> nit: Please use an anonymous namespace again instead of a static function.

Done.

[Toon Verwaest, 2016-06-20 14:41:35]

nice cleanup! lgtm

[adamk, 2016-06-20 16:29:53]

The CQ bit was checked by adamk@chromium.org

[adamk, 2016-06-20 16:29:53]

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/2061173002/#ps100001
(title: "Handled review comments")

[commit-bot: I haz the power, 2016-06-20 16:30:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2061173002/100001

[commit-bot: I haz the power, 2016-06-20 16:59:00]

Description was changed from

==========
[cleanup] Remove dead code from DeclareLookupSlot and rename it

Runtime_DeclareLookupSlot is used when generating code for var and function
declarations
originating in an eval. Over time, it's accumulated quite a bit of cruft, which
this CL removes:

  - With legacy const gone, lookup slots never have any property attributes.
  - There was a bit signaling that the variable was from an eval, but that was
redundant since
    DeclareLookupSlot is only used for eval.
  - Some Proxy-related code didn't make sense here.

Its name was also not terribly clear: while "LookupSlot" is used in several
places, this
particular function is only used for declaring variables and functions inside
sloppy eval.
Renamed (and split into two) to make this clear for future archeologists.

Also added various DCHECKs to check the assumptions being made.
==========

to

==========
[cleanup] Remove dead code from DeclareLookupSlot and rename it

Runtime_DeclareLookupSlot is used when generating code for var and function
declarations
originating in an eval. Over time, it's accumulated quite a bit of cruft, which
this CL removes:

  - With legacy const gone, lookup slots never have any property attributes.
  - There was a bit signaling that the variable was from an eval, but that was
redundant since
    DeclareLookupSlot is only used for eval.
  - Some Proxy-related code didn't make sense here.

Its name was also not terribly clear: while "LookupSlot" is used in several
places, this
particular function is only used for declaring variables and functions inside
sloppy eval.
Renamed (and split into two) to make this clear for future archeologists.

Also added various DCHECKs to check the assumptions being made.
==========

[commit-bot: I haz the power, 2016-06-20 16:59:01]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-20 17:00:57]

Description was changed from

==========
[cleanup] Remove dead code from DeclareLookupSlot and rename it

Runtime_DeclareLookupSlot is used when generating code for var and function
declarations
originating in an eval. Over time, it's accumulated quite a bit of cruft, which
this CL removes:

  - With legacy const gone, lookup slots never have any property attributes.
  - There was a bit signaling that the variable was from an eval, but that was
redundant since
    DeclareLookupSlot is only used for eval.
  - Some Proxy-related code didn't make sense here.

Its name was also not terribly clear: while "LookupSlot" is used in several
places, this
particular function is only used for declaring variables and functions inside
sloppy eval.
Renamed (and split into two) to make this clear for future archeologists.

Also added various DCHECKs to check the assumptions being made.
==========

to

==========
[cleanup] Remove dead code from DeclareLookupSlot and rename it

Runtime_DeclareLookupSlot is used when generating code for var and function
declarations
originating in an eval. Over time, it's accumulated quite a bit of cruft, which
this CL removes:

  - With legacy const gone, lookup slots never have any property attributes.
  - There was a bit signaling that the variable was from an eval, but that was
redundant since
    DeclareLookupSlot is only used for eval.
  - Some Proxy-related code didn't make sense here.

Its name was also not terribly clear: while "LookupSlot" is used in several
places, this
particular function is only used for declaring variables and functions inside
sloppy eval.
Renamed (and split into two) to make this clear for future archeologists.

Also added various DCHECKs to check the assumptions being made.

Committed: https://crrev.com/cbc6adc86cc2697785d311ff5bf9f736c7d9e725
Cr-Commit-Position: refs/heads/master@{#37111}
==========

[commit-bot: I haz the power, 2016-06-20 17:00:58]

Patchset 6 (id:??) landed as
https://crrev.com/cbc6adc86cc2697785d311ff5bf9f736c7d9e725
Cr-Commit-Position: refs/heads/master@{#37111}