thanks for the review. addressed comments. landing.
https://chromiumcodereview.appspot.com/10923003/diff/1/tools/grokdump.py File tools/grokdump.py (right): https://chromiumcodereview.appspot.com/10923003/diff/1/tools/grokdump.py#newcode36 tools/grokdump.py:36: import disasm On 2012/09/19 09:54:32, Michael Starzinger wrote:
I know you didn't touch, but can we alpha-sort the imports?
Done. https://chromiumcodereview.appspot.com/10923003/diff/1/tools/grokdump.py#newcode709 tools/grokdump.py:709: result = re.match(r"^FUNC ([a-f0-9]+) ([a-f0-9]+) ([a-f0-9]+) (.*)$", line) On 2012/09/19 09:54:32, Michael Starzinger wrote:
More than 80 characters.
Done. https://chromiumcodereview.appspot.com/10923003/diff/1/tools/grokdump.py#newcode714 tools/grokdump.py:714: bisect.insort_left(self.symbols, FuncSymbol(baseaddr + start, size, name)) On 2012/09/19 09:54:32, Michael Starzinger wrote:
More than 80 characters.
Done. https://chromiumcodereview.appspot.com/10923003/diff/1/tools/grokdump.py#newcode1770 tools/grokdump.py:1770: print On 2012/09/19 09:54:32, Michael Starzinger wrote:
Can we move this empty print to after the modules are printed?
I will add another one after modules. I like some empty lines, they make reading easier. https://chromiumcodereview.appspot.com/10923003/diff/1/tools/grokdump.py#newcode1771 tools/grokdump.py:1771: print " Modules:" On 2012/09/19 09:54:32, Michael Starzinger wrote:
Lowercase "modules:" would be consistent with the rest of the
exception info. Done. https://chromiumcodereview.appspot.com/10923003/diff/1/tools/grokdump.py#newcode1789 tools/grokdump.py:1789: print reader.FindSymbol(reader.ExceptionIP()) On 2012/09/19 09:54:32, Michael Starzinger wrote:
This will print "None" when no symbol is found, can we skip printing
in that
case? I think the "None" in the output is confusing.
Done. https://chromiumcodereview.appspot.com/10923003/diff/1/tools/grokdump.py#newcode1824 tools/grokdump.py:1824: maybe_symbol) On 2012/09/19 09:54:32, Michael Starzinger wrote:
Just use "maybe_symbol or ''" here and drop the if statement above.
Done. https://chromiumcodereview.appspot.com/10923003/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
