Strict mode parameter validation.

Strict mode parameter validation.
 * Duplicate parameter names
 * Parameters named "eval" or "arguments"


BUG=
TEST=test/mjsunit/strict-mode.js

[Lasse Reichstein, 2011-01-21 11:23:22]

LGTM. 
Simple and effective!

[Kevin Millikin (Chromium), 2011-01-21 11:41:56]

LGTM.  Superficial comments below.

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

http://codereview.chromium.org/6382006/diff/1/src/parser.cc#newcode3299
src/parser.cc:3299: Scanner::Location name_loc(RelocInfo::kNoPosition,
RelocInfo::kNoPosition);
This might be more readable with a factory function for "no location" in the
scanner class so you can write:

Scanner::Location name_loc = Scanner::NoLocation();

http://codereview.chromium.org/6382006/diff/1/src/parser.cc#newcode3307
src/parser.cc:3307: if (name_loc.beg_pos == RelocInfo::kNoPosition &&
Likewise, struct Scanner::Location could have a predicate for whether the
location is set so you could write:

if (name_loc.HasLocation() ...

If it seems to strange to ask a location if it has a location, we could try
something like Scanner::InvalidLocation() and Location::IsValid().

http://codereview.chromium.org/6382006/diff/1/src/parser.cc#newcode3309
src/parser.cc:3309: // Store location for later
This comment is probably redundant and so's the one in the body of the next if.

http://codereview.chromium.org/6382006/diff/1/src/parser.cc#newcode3313
src/parser.cc:3313: top_scope_->IsDeclared(param_name)) {
Indentation is off here.

http://codereview.chromium.org/6382006/diff/1/src/parser.cc#newcode3395
src/parser.cc:3395: ? function_token_position
We usually indent these continuations 4 spaces from the line above so save them
running away to the right:

int position = function_token_position != RelocInfo::kNoPosition
    ? function_token_position ....

[Martin Maly, 2011-01-21 23:08:39]

Thank you, Kevin. I incorporated your feedback. Please take another look.

Thanks!
Martin

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

http://codereview.chromium.org/6382006/diff/1/src/parser.cc#newcode3299
src/parser.cc:3299: Scanner::Location name_loc(RelocInfo::kNoPosition,
RelocInfo::kNoPosition);
On 2011/01/21 11:41:56, Kevin Millikin wrote:
> This might be more readable with a factory function for "no location" in the
> scanner class so you can write:
> 
> Scanner::Location name_loc = Scanner::NoLocation();

Done.

http://codereview.chromium.org/6382006/diff/1/src/parser.cc#newcode3307
src/parser.cc:3307: if (name_loc.beg_pos == RelocInfo::kNoPosition &&
On 2011/01/21 11:41:56, Kevin Millikin wrote:
> Likewise, struct Scanner::Location could have a predicate for whether the
> location is set so you could write:
> 
> if (name_loc.HasLocation() ...
> 
> If it seems to strange to ask a location if it has a location, we could try
> something like Scanner::InvalidLocation() and Location::IsValid().

Done. The Location::IsValid felt best so I went with that one.
The only slight glitch is that the scanner.h doesn't see assembler.h where
kNoPosition is located so the factory simply uses -1 literal.

http://codereview.chromium.org/6382006/diff/1/src/parser.cc#newcode3309
src/parser.cc:3309: // Store location for later
On 2011/01/21 11:41:56, Kevin Millikin wrote:
> This comment is probably redundant and so's the one in the body of the next
if.

Done.

http://codereview.chromium.org/6382006/diff/1/src/parser.cc#newcode3313
src/parser.cc:3313: top_scope_->IsDeclared(param_name)) {
On 2011/01/21 11:41:56, Kevin Millikin wrote:
> Indentation is off here.

Done.

http://codereview.chromium.org/6382006/diff/1/src/parser.cc#newcode3395
src/parser.cc:3395: ? function_token_position
On 2011/01/21 11:41:56, Kevin Millikin wrote:
> We usually indent these continuations 4 spaces from the line above so save
them
> running away to the right:
> 
> int position = function_token_position != RelocInfo::kNoPosition
>     ? function_token_position ....

Done.

[Lasse Reichstein, 2011-01-25 13:03:55]

LGTM with comments addressed.

http://codereview.chromium.org/6382006/diff/6001/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/6382006/diff/6001/src/parser.cc#newcode3398
src/parser.cc:3398: if (name_loc.beg_pos != RelocInfo::kNoPosition) {
Use name_loc.IsValid() here and below.

http://codereview.chromium.org/6382006/diff/6001/src/scanner-base.h
File src/scanner-base.h (right):

http://codereview.chromium.org/6382006/diff/6001/src/scanner-base.h#newcode276
src/scanner-base.h:276: return beg_pos >= 0 && end_pos >= 0;
How about ... && end_pos >= beg_pos;
Seems more "valid" to me :)

http://codereview.chromium.org/6382006/diff/6001/test/mjsunit/strict-mode.js
File test/mjsunit/strict-mode.js (right):

http://codereview.chromium.org/6382006/diff/6001/test/mjsunit/strict-mode.js#...
test/mjsunit/strict-mode.js:91: CheckStrictMode("function foo(a, b, c, d, b)
{}", SyntaxError)
How about checking that it's not an error in non-strict mode?

[Martin Maly, 2011-01-25 17:21:25]

Done on all accounts. The test stays unchanged because the function
CheckStrictMode already checks that the code works outside of strict mode and
triggers SyntaxError in strict mode.

Thank you!
Martin

http://codereview.chromium.org/6382006/diff/6001/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/6382006/diff/6001/src/parser.cc#newcode3398
src/parser.cc:3398: if (name_loc.beg_pos != RelocInfo::kNoPosition) {
On 2011/01/25 13:03:55, Lasse Reichstein wrote:
> Use name_loc.IsValid() here and below.

Done.

http://codereview.chromium.org/6382006/diff/6001/src/scanner-base.h
File src/scanner-base.h (right):

http://codereview.chromium.org/6382006/diff/6001/src/scanner-base.h#newcode276
src/scanner-base.h:276: return beg_pos >= 0 && end_pos >= 0;
On 2011/01/25 13:03:55, Lasse Reichstein wrote:
> How about ... && end_pos >= beg_pos;
> Seems more "valid" to me :)

Done. Using beg_pos >= 0 && end_pos >= beg_pos

because end_pos >= beg_pos >= 0 ===> end_pos >= 0

http://codereview.chromium.org/6382006/diff/6001/test/mjsunit/strict-mode.js
File test/mjsunit/strict-mode.js (right):

http://codereview.chromium.org/6382006/diff/6001/test/mjsunit/strict-mode.js#...
test/mjsunit/strict-mode.js:91: CheckStrictMode("function foo(a, b, c, d, b)
{}", SyntaxError)
On 2011/01/25 13:03:55, Lasse Reichstein wrote:
> How about checking that it's not an error in non-strict mode?

Already implemented. The CheckStrictMode function will compile the snippet
without strict mode and then in several configuration of strict mode.