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.
http://codereview.chromium.org/215052
--~--~---------~--~----~------------~-------~--~----~
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
-~----------~----~----~----~------~----~------~--~---
