- Fixed the formatting.
- Ran the pages through an HTML validator and fixed the errors.
- There is still some bug in disasm view, will fix in the next patchset
(column
splitting/objdump parsing, address annotations).
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 = [ ]
On 2014/03/24 15:21:32, Jakob wrote:
nit: the style guide doesn't like spaces inside empty pairs of (), [],
{}.
Done.
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]+)
(.*)$")
On 2014/03/24 15:21:32, Jakob wrote:
style nit: two empty lines between top-level things (no empty line
between
constants is fine)
Done.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1603
tools/grokdump.py:1603: f = open(self.cmt_file, "r")
On 2014/03/24 15:21:32, Jakob wrote:
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.
Done.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1624
tools/grokdump.py:1624: f = open(self.cmt_file, "a")
On 2014/03/24 15:21:32, Jakob wrote:
again, prefer "with"
Done.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1629
tools/grokdump.py:1629: self.styles = { }
On 2014/03/24 15:21:32, Jakob wrote:
nit: I have a weak preference to initialize all member variables in
the
constructor (i.e. __init__).
Done.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1631
tools/grokdump.py:1631: # Color all stack addresses
On 2014/03/24 15:21:32, Jakob wrote:
nit: trailing full stop please
Done.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1660
tools/grokdump.py:1660: f = open(self.cmt_file, "a")
On 2014/03/24 15:21:32, Jakob wrote:
with
Done.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1767
tools/grokdump.py:1767: .code {{
On 2014/03/24 15:21:32, Jakob wrote:
If you used %s-placeholders ("foo %s baz" % bar), you wouldn't need to
duplicate
all those curly braces... up to you.
However, with %s you cannot reuse the same argument for several
replacements.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1768
tools/grokdump.py:1768: font-family: monospace;
On 2014/03/24 15:21:32, Jakob wrote:
nit: funky indentation
Done.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1832
tools/grokdump.py:1832: }}
On 2014/03/24 15:21:32, Jakob wrote:
Is this intentionally empty?
Removed.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1891
tools/grokdump.py:1891: xmlhttp.open("GET",
On 2014/03/24 15:21:32, Jakob wrote:
I think using GET to perform actions is considered bad style... then
again I
guess in this case we don't care.
Yeah, I am leaving as is.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1924
tools/grokdump.py:1924: location.reload(true)
On 2014/03/24 15:21:32, Jakob wrote:
nit: {} around the block please
Done.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1932
tools/grokdump.py:1932: function main() {{
On 2014/03/24 15:21:32, Jakob wrote:
looks like we don't need this urgently.
Removed.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1962
tools/grokdump.py:1962: <!--
On 2014/03/24 15:21:32, Jakob wrote:
nit: remove this if it's not needed
Done.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1980
tools/grokdump.py:1980: class WebParameterError(Exception):
On 2014/03/24 15:21:32, Jakob wrote:
again, two empty lines before and after top-level classes please
Done.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1985
tools/grokdump.py:1985: def fmt(self, qc):
On 2014/03/24 15:21:32, Jakob wrote:
naming nit: abbreviations are generally frowned upon. cmt, fmt, qc,
wtf?
Done.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2000
tools/grokdump.py:2000: print "GET!"
On 2014/03/24 15:21:32, Jakob wrote:
Looks like a debugging aid. Is this still needed?
Removed.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2057
tools/grokdump.py:2057: self.send_error(404,'Invalid params')
On 2014/03/24 15:21:32, Jakob wrote:
again, I'd prefer if+else over if+return
Done.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2132
tools/grokdump.py:2132: print "Adding comment to dump '%s': %s -> '%s'"
% (
On 2014/03/24 15:21:32, Jakob wrote:
Still needed? If you don't want to delete it, consider putting it
behind the
existing "if DEBUG:" global flag.
Removed.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2141
tools/grokdump.py:2141: print "Adding page annotation to dump '%s': %s
-> %s" % (
On 2014/03/24 15:21:32, Jakob wrote:
still needed?
Removed.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2268
tools/grokdump.py:2268: f.write(" Exception code: %08X\n" %
(
On 2014/03/24 15:21:32, Jakob wrote:
nit: I'd break as:
f.write(" Exception code: %08X\n" %
self.reader.exception.exception.code)
(Again below.)
Done.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2295
tools/grokdump.py:2295: (a + size - 1) % size)
On 2014/03/24 15:21:32, Jakob wrote:
nit: fits on previous line?
Done.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2299
tools/grokdump.py:2299: def format_object(self, address):
On 2014/03/24 15:21:32, Jakob wrote:
nit: one empty line is enough between non-toplevel definitions
Done.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2323
tools/grokdump.py:2323: def output_words(self, f, start_address,
end_address, \
On 2014/03/24 15:21:32, Jakob wrote:
nit: trailing \ is not necessary
Done.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2353
tools/grokdump.py:2353: end_address,
On 2014/03/24 15:21:32, Jakob wrote:
nit: fits on one line?
Done.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2442
tools/grokdump.py:2442: f.write("·")
On 2014/03/24 15:21:32, Jakob wrote:
Doesn't this need s/·/·/ ?
Done (the char code above needs ';', too)
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2592
tools/grokdump.py:2592:
On 2014/03/24 15:21:32, Jakob wrote:
nit: just one empty line
Done.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2746
tools/grokdump.py:2746: f.write(("<input type=\"text\"
class=\"dumpcomments\" "
On 2014/03/24 15:21:32, Jakob wrote:
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))
Done.
https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode3157
tools/grokdump.py:3157: help="start a web page")
On 2014/03/24 15:21:32, Jakob wrote:
nit: s/page/server on localhost:8081/ (is it worth the effort to make
the port
configurable?)
Done (the port not configurable yet)
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.
