LGTM, just nits. Great stuff!
I haven't spent much time verifying correctness. We'll notice
in "production"
when something doesn't work.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py
File tools/grokdump.py (right):
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode714
tools/grokdump.py:714: aligned_res = [ ]
nit: the style guide doesn't like spaces inside empty pairs of (), [],
{}.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1592
tools/grokdump.py:1592: COMMENT_RE = re.compile(r"^C (0x[0-9a-fA-F]+)
(.*)$")
style nit: two empty lines between top-level things (no empty line
between constants is fine)
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1603
tools/grokdump.py:1603: f = open(self.cmt_file, "r")
The pythonic way to express open/close sequences is:
with open(filename, "r") as f:
lines = f.readlines()
The beauty being that f is automatically closed at the end of the
with-block regardless of exceptions being thrown or whatever else
happening.
Consider replacing the try..except construct with "if
os.path.exists(self.cmt_file):". Arguably it is good style to avoid
exceptions for commonplace control flow.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1624
tools/grokdump.py:1624: f = open(self.cmt_file, "a")
again, prefer "with"
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1629
tools/grokdump.py:1629: self.styles = { }
nit: I have a weak preference to initialize all member variables in the
constructor (i.e. __init__).
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1631
tools/grokdump.py:1631: # Color all stack addresses
nit: trailing full stop please
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1660
tools/grokdump.py:1660: f = open(self.cmt_file, "a")
with
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1767
tools/grokdump.py:1767: .code {{
If you used %s-placeholders ("foo %s baz" % bar), you wouldn't need to
duplicate all those curly braces... up to you.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1768
tools/grokdump.py:1768: font-family: monospace;
nit: funky indentation
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1832
tools/grokdump.py:1832: }}
Is this intentionally empty?
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1891
tools/grokdump.py:1891: xmlhttp.open("GET",
I think using GET to perform actions is considered bad style... then
again I guess in this case we don't care.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1924
tools/grokdump.py:1924: location.reload(true)
nit: {} around the block please
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1932
tools/grokdump.py:1932: function main() {{
looks like we don't need this urgently.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1962
tools/grokdump.py:1962: <!--
nit: remove this if it's not needed
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1980
tools/grokdump.py:1980: class WebParameterError(Exception):
again, two empty lines before and after top-level classes please
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1985
tools/grokdump.py:1985: def fmt(self, qc):
naming nit: abbreviations are generally frowned upon. cmt, fmt, qc, wtf?
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2000
tools/grokdump.py:2000: print "GET!"
Looks like a debugging aid. Is this still needed?
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2007
tools/grokdump.py:2007: if parsedurl.path == "/summary.html":
s/if/elif/ like below? In turn you could omit all the "return"
statements.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2057
tools/grokdump.py:2057: self.send_error(404,'Invalid params')
again, I'd prefer if+else over if+return
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2112
tools/grokdump.py:2112: stack_bottom = exception_thread.stack.start + \
nit: the style guide prefers () over trailing \, i.e.:
stack_bottom = (exception_thread.stack.start +
exception_thread.stack.memory.data_size)
Again below.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2132
tools/grokdump.py:2132: print "Adding comment to dump '%s': %s -> '%s'"
% (
Still needed? If you don't want to delete it, consider putting it behind
the existing "if DEBUG:" global flag.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2141
tools/grokdump.py:2141: print "Adding page annotation to dump '%s': %s
-> %s" % (
still needed?
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2158
tools/grokdump.py:2158: if maybeaddress == None:
nit: the style guide wants "is None" instead of "== None". Also,
"maybe_address" would be a tad more readable.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2268
tools/grokdump.py:2268: f.write(" Exception code: %08X\n" %
(
nit: I'd break as:
f.write(" Exception code: %08X\n" %
self.reader.exception.exception.code)
(Again below.)
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2295
tools/grokdump.py:2295: (a + size - 1) % size)
nit: fits on previous line?
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2299
tools/grokdump.py:2299: def format_object(self, address):
nit: one empty line is enough between non-toplevel definitions
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2323
tools/grokdump.py:2323: def output_words(self, f, start_address,
end_address, \
nit: trailing \ is not necessary
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2353
tools/grokdump.py:2353: end_address,
nit: fits on one line?
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2442
tools/grokdump.py:2442: f.write("·")
Doesn't this need s/·/·/ ?
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2592
tools/grokdump.py:2592:
nit: just one empty line
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2746
tools/grokdump.py:2746: f.write(("<input type=\"text\"
class=\"dumpcomments\" "
nit: the indentation here is particularly inconsistent. Suggestion:
f.write("<input type=\"text\" class=\"dumpcomments\" "
"id=\"dump-%s\" onchange=\"dump_cmt()\" value=\"%s\">\n" %
(cgi.escape(name), desc))
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode3157
tools/grokdump.py:3157: help="start a web page")
nit: s/page/server on localhost:8081/ (is it worth the effort to make
the port configurable?)
https://codereview.chromium.org/194573005/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
