Replace boolean indications of strict mode by an enum value.

Replace boolean indications of strict mode by an enum value.

Committed: http://code.google.com/p/v8/source/detail?r=9746

[Steven, 2011-10-20 13:35:06]

PTAL.

[Yang, 2011-10-20 14:18:59]

LGTM if comments address non-issues.

http://codereview.chromium.org/8344082/diff/1/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/8344082/diff/1/src/compiler.cc#newcode602
src/compiler.cc:602: shared->set_strict_mode_flag(strict_mode);
This is not quite equivalent, I guess. The old version only sets if in strict
mode, the new version sets in any case. Also, any performance implications?

http://codereview.chromium.org/8344082/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/8344082/diff/1/src/parser.cc#newcode655
src/parser.cc:655: top_scope_->SetStrictModeFlag(strict_mode);
Here again. This only works if you assume the previous state to be not in strict
mode.

http://codereview.chromium.org/8344082/diff/1/src/parser.cc#newcode742
src/parser.cc:742: scope->SetStrictModeFlag(shared_info->strict_mode_flag());
Ditto.

[Steven, 2011-10-21 13:59:00]

Can you please take a look again.

http://codereview.chromium.org/8344082/diff/1/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/8344082/diff/1/src/compiler.cc#newcode602
src/compiler.cc:602: shared->set_strict_mode_flag(strict_mode);
Testing again if the parsed function literal is in strict mode. However, this
should have been a non-issue because the update would only happen if we didn't
know the mode before and we figured it out for the first time by parsing in
which case the value from the function literal is correct.

On 2011/10/20 14:18:59, Yang wrote:
> This is not quite equivalent, I guess. The old version only sets if in strict
> mode, the new version sets in any case. Also, any performance implications?

http://codereview.chromium.org/8344082/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/8344082/diff/1/src/parser.cc#newcode655
src/parser.cc:655: top_scope_->SetStrictModeFlag(strict_mode);
Because we start with a GLOBAL or EVAL scope the previous scope is null and
therefore no the created scope won't be in strict mode.
On 2011/10/20 14:18:59, Yang wrote:
> Here again. This only works if you assume the previous state to be not in
strict
> mode.

http://codereview.chromium.org/8344082/diff/1/src/parser.cc#newcode742
src/parser.cc:742: scope->SetStrictModeFlag(shared_info->strict_mode_flag());
The move before the if was not correct. I added a test case which catches this.
Always setting the StrictModeFlag here is a non-issue, however for non-trivial
reasons. Added ASSERTS to verify this. As far as I can see these are the
possible cases
* the SharedFunctionInfo has the correct value: this  function is non-strict or
outer scope is strict or the function was preparsed
* the outer scope is non-strict and the non-preparsed  strict function is being
parsed for the first time. In this case ParseFunctionLiteral will determine the
correct mode and Compiler::CompileLazy updates the SharedFunctionInfo to the
correct value.

On 2011/10/20 14:18:59, Yang wrote:
> Ditto.

[Yang, 2011-10-21 15:25:45]

On 2011/10/21 13:59:00, Steven wrote:
> Can you please take a look again.
> 
> http://codereview.chromium.org/8344082/diff/1/src/compiler.cc
> File src/compiler.cc (right):
> 
> http://codereview.chromium.org/8344082/diff/1/src/compiler.cc#newcode602
> src/compiler.cc:602: shared->set_strict_mode_flag(strict_mode);
> Testing again if the parsed function literal is in strict mode. However, this
> should have been a non-issue because the update would only happen if we didn't
> know the mode before and we figured it out for the first time by parsing in
> which case the value from the function literal is correct.
> 
> On 2011/10/20 14:18:59, Yang wrote:
> > This is not quite equivalent, I guess. The old version only sets if in
strict
> > mode, the new version sets in any case. Also, any performance implications?
> 
> http://codereview.chromium.org/8344082/diff/1/src/parser.cc
> File src/parser.cc (right):
> 
> http://codereview.chromium.org/8344082/diff/1/src/parser.cc#newcode655
> src/parser.cc:655: top_scope_->SetStrictModeFlag(strict_mode);
> Because we start with a GLOBAL or EVAL scope the previous scope is null and
> therefore no the created scope won't be in strict mode.
> On 2011/10/20 14:18:59, Yang wrote:
> > Here again. This only works if you assume the previous state to be not in
> strict
> > mode.
> 
> http://codereview.chromium.org/8344082/diff/1/src/parser.cc#newcode742
> src/parser.cc:742: scope->SetStrictModeFlag(shared_info->strict_mode_flag());
> The move before the if was not correct. I added a test case which catches
this.
> Always setting the StrictModeFlag here is a non-issue, however for non-trivial
> reasons. Added ASSERTS to verify this. As far as I can see these are the
> possible cases
> * the SharedFunctionInfo has the correct value: this  function is non-strict
or
> outer scope is strict or the function was preparsed
> * the outer scope is non-strict and the non-preparsed  strict function is
being
> parsed for the first time. In this case ParseFunctionLiteral will determine
the
> correct mode and Compiler::CompileLazy updates the SharedFunctionInfo to the
> correct value.
> 
> On 2011/10/20 14:18:59, Yang wrote:
> > Ditto.

LGTM if all tests pass.

[Yang, 2011-10-21 15:25:59]

forgot to send comment.

http://codereview.chromium.org/8344082/diff/8003/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/8344082/diff/8003/src/compiler.cc#newcode602
src/compiler.cc:602: if (strict_mode != kNonStrictMode) {
With the assertions in place, the if clause can actually be omitted now.