Do not ignore an empty context with extension when creating a scope object.

Do not ignore an empty context with extension when creating a scope object.

Runtime_DebugEvaluate creates an empty context which is not correctly handled in FullCodeGenerator::ContextSlotOperandCheckExtensions because the corresponding scope indicates that it has no context.

BUG=crbug.com/107996
TEST=test/mjsunit/regress/regress-crbug-107996.js

Committed: https://code.google.com/p/v8/source/detail?r=10582

[ulan, 2012-02-01 16:06:54]

Could please take a look?

[Kevin Millikin (Chromium), 2012-02-02 08:36:13]

There might be a simpler and safer way:

https://chromiumcodereview.appspot.com/9310027/diff/1/src/scopes.cc
File src/scopes.cc (left):

https://chromiumcodereview.appspot.com/9310027/diff/1/src/scopes.cc#oldcode152
src/scopes.cc:152: if (*scope_info != ScopeInfo::Empty()) {
As a simple cleanup, you can get rid of this completely (SetDefaults already set
language_mode for all scopes, no need to overwrite it---unless I'm reading the
code wrong).

https://chromiumcodereview.appspot.com/9310027/diff/1/src/scopes.cc
File src/scopes.cc (right):

https://chromiumcodereview.appspot.com/9310027/diff/1/src/scopes.cc#newcode151
src/scopes.cc:151: if (!scope_info.is_null() && *scope_info !=
ScopeInfo::Empty()) {
As far as I can tell, there is no reason *at all* that this function should ever
produce a scope indicating there's no context---because there is definitely a
materialized context.  I think it's much more straightforward to write this
function as:

SetDefaults(type, NULL, scope_info);
if (!scope_info.is_null()) {
  num_heap_slots_ = scope_info->ContextLength();
}
// Ensure at least MIN_CONTEXT_SLOTS to indicate a materialized context.
num_heap_slots_ = Max(num_heap_slots_, Context::MIN_CONTEXT_SLOTS);
AddInnerScope(inner_scope);

And no need for the default-valued parameter.  Does that implementation make
sense?

https://chromiumcodereview.appspot.com/9310027/diff/1/src/scopes.h
File src/scopes.h (right):

https://chromiumcodereview.appspot.com/9310027/diff/1/src/scopes.h#newcode549
src/scopes.h:549: bool has_context_with_extension = false);
Please no: default parameters are bad, boolean-valued default parameters are
really bad.

[ulan, 2012-02-02 09:16:19]

Thank you, Kevin. This way is much cleaner. I uploaded new patch set.

https://chromiumcodereview.appspot.com/9310027/diff/1/src/scopes.cc
File src/scopes.cc (right):

https://chromiumcodereview.appspot.com/9310027/diff/1/src/scopes.cc#newcode151
src/scopes.cc:151: if (!scope_info.is_null() && *scope_info !=
ScopeInfo::Empty()) {
On 2012/02/02 08:36:13, Kevin Millikin wrote:
> As far as I can tell, there is no reason *at all* that this function should
ever
> produce a scope indicating there's no context---because there is definitely a
> materialized context.  I think it's much more straightforward to write this
> function as:
> 
> SetDefaults(type, NULL, scope_info);
> if (!scope_info.is_null()) {
>   num_heap_slots_ = scope_info->ContextLength();
> }
> // Ensure at least MIN_CONTEXT_SLOTS to indicate a materialized context.
> num_heap_slots_ = Max(num_heap_slots_, Context::MIN_CONTEXT_SLOTS);
> AddInnerScope(inner_scope);
> 
> And no need for the default-valued parameter.  Does that implementation make
> sense?

Indeed, this constructor is called only from DeserializeScopeChain.

[Kevin Millikin (Chromium), 2012-02-02 09:21:13]

LGTM!