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