* Remove non-Open Source code from Douglas Crockford....

* Remove non-Open Source code from Douglas Crockford.
* Be more var-correct in JS files.
* Rename some JS variables to reflect the fact that they are instance
  variables on the global intrinsics object.
* Missing optimization in StringCharAt.

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

[Christian Plesner Hansen, 2009-09-22 12:50:31]

Lgtm, with a load of python comments.

http://codereview.chromium.org/215052/diff/1/11
File src/string.js (right):

http://codereview.chromium.org/215052/diff/1/11#newcode166
Line 166: // last_match_info_ is defined in regexp-delay.js.
This should be camelCase right?

http://codereview.chromium.org/215052/diff/1/11#newcode373
Line 373: var last_match_info = DoRegExpExec(regexp, subject, 0);
Please consider renaming this to something that is more descriptive, or at least
differs from last_match_info_ by more than just an underscore.  Also, camelCase.

http://codereview.chromium.org/215052/diff/1/3
File tools/jsmin.py (right):

http://codereview.chromium.org/215052/diff/1/3#newcode1
Line 1: #!/usr/bin/env python
This file is a noop when executed anyway so why not just drop this?

http://codereview.chromium.org/215052/diff/1/3#newcode30
Line 30: # This is a JavaScript minifier, written in Python.
Written in python is sort of implied by .py.

http://codereview.chromium.org/215052/diff/1/3#newcode40
Line 40: # deeper curly brace level than the uses.
Module documentation should be a docstring.  See under 'Comments' in
http://www.corp.google.com/eng/doc/pyguide.xml.

http://codereview.chromium.org/215052/diff/1/3#newcode46
Line 46: # program2 = minifier.JSMinify(program2)
Ditto, combine with the above in a docstring.

http://codereview.chromium.org/215052/diff/1/3#newcode68
Line 68: self.seen_identifiers[identifier] = 1
Why 1 and not True?

http://codereview.chromium.org/215052/diff/1/3#newcode70
Line 70: # Called on matching interesting bits of the program.  These can be
curly
Since you're writing comments anyway why not use the docstring syntax?

http://codereview.chromium.org/215052/diff/1/3#newcode78
Line 78: self.maps.append({})
If it always holds that self.nesting == len(self.maps) - 1 then you could use a
self.nesting() accessor instead and save some bookkeeping.

http://codereview.chromium.org/215052/diff/1/3#newcode79
Line 79: self.maps[self.nesting][".old_identifier_counter"] = (
Why not use a proper object with a proper field rather than this hack?

http://codereview.chromium.org/215052/diff/1/3#newcode88
Line 88: if re.match("[\"'/]", matched_text):
Prefix regexp source string with r.

http://codereview.chromium.org/215052/diff/1/3#newcode95
Line 95: m = re.match(r"(function\b[^(]*)\((.*)\)\{$", matched_text)
Should there be a \b before the word 'function'?

http://codereview.chromium.org/215052/diff/1/3#newcode102
Line 102: self.maps[self.nesting][".old_identifier_counter"] = (
I've seen this code twice now.  How about factoring it into a method?

http://codereview.chromium.org/215052/diff/1/3#newcode106
Line 106: for i in range(self.nesting, -1, -1):
'range' materializes the array you're iterating through -- use 'xrange' instead.

http://codereview.chromium.org/215052/diff/1/3#newcode160
Line 160: #line = line.replace("\t", " ")
Why is this commented out?

http://codereview.chromium.org/215052/diff/1/3#newcode176
Line 176: self.in_comment = 1
Use True instead of 1.

http://codereview.chromium.org/215052/diff/1/3#newcode184
Line 184: double_quoted_string = '"(?:[^"\\\\]|\\\\.)*"'
Remember the r, r'...'.

http://codereview.chromium.org/215052/diff/1/3#newcode212
Line 212: line = re.sub(double_quoted_string + "|" +
Instead of manually '+ "|"'-ing you could collect the terms in a list and do
"|".join([...]).

[Erik Corry, 2009-09-23 12:04:37]

I uploaded a new version with the fixes.  I also changed the algorithm.  It now
does only one scope per top-level {} instead of introducing a new scope for each
inner {}.  I think that's less likely to cause accidental semantic changes to
the code.  I also stopped renaming variables on the top level (on the global
object).  This means the ends-in-underscore hack is gone.  The two changes
combined cost us 544 bytes, which should be acceptable.

http://codereview.chromium.org/215052/diff/1/11
File src/string.js (right):

http://codereview.chromium.org/215052/diff/1/11#newcode166
Line 166: // last_match_info_ is defined in regexp-delay.js.
On 2009/09/22 12:50:31, Christian Plesner Hansen wrote:
> This should be camelCase right?

Fixed.

http://codereview.chromium.org/215052/diff/1/11#newcode373
Line 373: var last_match_info = DoRegExpExec(regexp, subject, 0);
On 2009/09/22 12:50:31, Christian Plesner Hansen wrote:
> Please consider renaming this to something that is more descriptive, or at
least
> differs from last_match_info_ by more than just an underscore.  Also,
camelCase.

Done.

http://codereview.chromium.org/215052/diff/1/3
File tools/jsmin.py (right):

http://codereview.chromium.org/215052/diff/1/3#newcode1
Line 1: #!/usr/bin/env python
On 2009/09/22 12:50:31, Christian Plesner Hansen wrote:
> This file is a noop when executed anyway so why not just drop this?

I'm going to leave it in since it is used by syntax colouring editors.

http://codereview.chromium.org/215052/diff/1/3#newcode30
Line 30: # This is a JavaScript minifier, written in Python.
On 2009/09/22 12:50:31, Christian Plesner Hansen wrote:
> Written in python is sort of implied by .py.

Removed.

http://codereview.chromium.org/215052/diff/1/3#newcode40
Line 40: # deeper curly brace level than the uses.
On 2009/09/22 12:50:31, Christian Plesner Hansen wrote:
> Module documentation should be a docstring.  See under 'Comments' in
> http://www.corp.google.com/eng/doc/pyguide.xml.

Fixed.

http://codereview.chromium.org/215052/diff/1/3#newcode46
Line 46: # program2 = minifier.JSMinify(program2)
On 2009/09/22 12:50:31, Christian Plesner Hansen wrote:
> Ditto, combine with the above in a docstring.

Done.

http://codereview.chromium.org/215052/diff/1/3#newcode68
Line 68: self.seen_identifiers[identifier] = 1
On 2009/09/22 12:50:31, Christian Plesner Hansen wrote:
> Why 1 and not True?

Fixed.

http://codereview.chromium.org/215052/diff/1/3#newcode70
Line 70: # Called on matching interesting bits of the program.  These can be
curly
On 2009/09/22 12:50:31, Christian Plesner Hansen wrote:
> Since you're writing comments anyway why not use the docstring syntax?

Done.

http://codereview.chromium.org/215052/diff/1/3#newcode78
Line 78: self.maps.append({})
On 2009/09/22 12:50:31, Christian Plesner Hansen wrote:
> If it always holds that self.nesting == len(self.maps) - 1 then you could use
a
> self.nesting() accessor instead and save some bookkeeping.

This issue has disappeared.

http://codereview.chromium.org/215052/diff/1/3#newcode79
Line 79: self.maps[self.nesting][".old_identifier_counter"] = (
On 2009/09/22 12:50:31, Christian Plesner Hansen wrote:
> Why not use a proper object with a proper field rather than this hack?

This issue has disappeared.

http://codereview.chromium.org/215052/diff/1/3#newcode88
Line 88: if re.match("[\"'/]", matched_text):
On 2009/09/22 12:50:31, Christian Plesner Hansen wrote:
> Prefix regexp source string with r.

Strangely enough this doesn't work.  The literal r"[\"'/]" puts a backslash
character in the string, which is not what I want.  Lasse suggests r"""["'/]"""
which works, but I feel it's uglier than what I had before.

http://codereview.chromium.org/215052/diff/1/3#newcode95
Line 95: m = re.match(r"(function\b[^(]*)\((.*)\)\{$", matched_text)
On 2009/09/22 12:50:31, Christian Plesner Hansen wrote:
> Should there be a \b before the word 'function'?

match() implies ^ which in this case implies \b.

http://codereview.chromium.org/215052/diff/1/3#newcode102
Line 102: self.maps[self.nesting][".old_identifier_counter"] = (
On 2009/09/22 12:50:31, Christian Plesner Hansen wrote:
> I've seen this code twice now.  How about factoring it into a method?

Fixed.

http://codereview.chromium.org/215052/diff/1/3#newcode106
Line 106: for i in range(self.nesting, -1, -1):
On 2009/09/22 12:50:31, Christian Plesner Hansen wrote:
> 'range' materializes the array you're iterating through -- use 'xrange'
instead.

This issue has disappeared.

http://codereview.chromium.org/215052/diff/1/3#newcode160
Line 160: #line = line.replace("\t", " ")
On 2009/09/22 12:50:31, Christian Plesner Hansen wrote:
> Why is this commented out?

Fixed.

http://codereview.chromium.org/215052/diff/1/3#newcode176
Line 176: self.in_comment = 1
On 2009/09/22 12:50:31, Christian Plesner Hansen wrote:
> Use True instead of 1.

Fixed.

http://codereview.chromium.org/215052/diff/1/3#newcode184
Line 184: double_quoted_string = '"(?:[^"\\\\]|\\\\.)*"'
On 2009/09/22 12:50:31, Christian Plesner Hansen wrote:
> Remember the r, r'...'.

Fixed.

http://codereview.chromium.org/215052/diff/1/3#newcode212
Line 212: line = re.sub(double_quoted_string + "|" +
On 2009/09/22 12:50:31, Christian Plesner Hansen wrote:
> Instead of manually '+ "|"'-ing you could collect the terms in a list and do
> "|".join([...]).

Fixed.

[Christian Plesner Hansen, 2009-09-23 12:19:35]

Lgtm!

http://codereview.chromium.org/215052/diff/4001/4003
File tools/jsmin.py (right):

http://codereview.chromium.org/215052/diff/4001/4003#newcode30
Line 30: # Suppress copyright warning: pylint: disable-msg=C6304
I would suggest removing this and just letting pylint complain.

http://codereview.chromium.org/215052/diff/4001/4003#newcode125
Line 125: # Enters it into the mapping table for this scope.
Why is this not part of the docstring?