Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(268)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 11363163: Utility Graphviz-create.py. It looks at hydrogen.cfg files and creates graphviz (dot) output of var…

Created:
8 years, 1 month ago by mvstanton
Modified:
7 years, 11 months ago
Reviewers:
Jakob Kummerow
CC:
v8-dev
Visibility:
Public.

Description

Utility Graphviz-create.py. It looks at hydrogen.cfg files and creates graphviz (dot) output of various types. BUG=

Patch Set 1 #

Patch Set 2 : Some improvements from comments #

Patch Set 3 : Removed hard-coded list of hydrogen phases. #

Total comments: 22

Patch Set 4 : Review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+698 lines, -0 lines) Patch
A tools/graphviz-create.py View 1 2 3 1 chunk +698 lines, -0 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
mvstanton
Hi guys, here is the graphviz utility for looking at hydrogen.cfg files in different (and ...
8 years, 1 month ago (2012-11-09 08:33:51 UTC) #1
mvstanton
Hi Jakob, thx for comments. 1) formatting addressed, 2) removed hard-coded Hydrogen phase list.
8 years, 1 month ago (2012-11-14 12:22:12 UTC) #2
Jakob Kummerow
There are more style issues here... please apply the same rules you would apply to ...
8 years, 1 month ago (2012-11-16 17:31:57 UTC) #3
mvstanton
7 years, 11 months ago (2013-01-06 11:26:36 UTC) #4 
Hi Jakob, I finally returned to this. Thanks for the helpful comments!

https://codereview.chromium.org/11363163/diff/7001/tools/graphviz-create.py
File tools/graphviz-create.py (right):

https://codereview.chromium.org/11363163/diff/7001/tools/graphviz-create.py#n...
tools/graphviz-create.py:35: import shlex
On 2012/11/16 17:31:57, Jakob wrote:
> nit: sort imports alphabetically please

Done.

https://codereview.chromium.org/11363163/diff/7001/tools/graphviz-create.py#n...
tools/graphviz-create.py:89: def die(inputobj,expected):
On 2012/11/16 17:31:57, Jakob wrote:
> Looking at what this method does, shouldn't it be called ASSERT_EQ(expected,
> actual)? In that case, why not use Python's builtin "assert" statement?

thx. I changed to use assert. I kept diemsg() as a way to spit out a formatted
message and end process.

https://codereview.chromium.org/11363163/diff/7001/tools/graphviz-create.py#n...
tools/graphviz-create.py:104: def CreateFile(self,filename):
On 2012/11/16 17:31:57, Jakob wrote:
> Create/close pairs are brittle. The most robust way to work with files in
Python
> is to use with-blocks (which auto-close the file when they go out of scope),
> e.g.:
> 
> with open(filename, "w") as f:
>   f.write("stuff")
> 
> I think I'd remove the Printer class and just pass around opened file objects
> that are created in such a with-statement. The code at use sites should be
> almost identical.

Done.

https://codereview.chromium.org/11363163/diff/7001/tools/graphviz-create.py#n...
tools/graphviz-create.py:169: self.name,
On 2012/11/16 17:31:57, Jakob wrote:
> 4-space indentation when you break inside ()

Done.

https://codereview.chromium.org/11363163/diff/7001/tools/graphviz-create.py#n...
tools/graphviz-create.py:257: help="Input file (default hydrogen.cfg)",
On 2012/11/16 17:31:57, Jakob wrote:
> No need to type the default value twice:
> 
> help="Input file (default: %default)"
> 
> See http://docs.python.org/2/library/optparse.html#generating-help

Done.

https://codereview.chromium.org/11363163/diff/7001/tools/graphviz-create.py#n...
tools/graphviz-create.py:260: help="The command to run: 'diff','report','blocks'
(default)",
On 2012/11/16 17:31:57, Jakob wrote:
> nit: long line

Done.

https://codereview.chromium.org/11363163/diff/7001/tools/graphviz-create.py#n...
tools/graphviz-create.py:553: 
On 2012/11/16 17:31:57, Jakob wrote:
> nit: one empty line is enough

Done.

https://codereview.chromium.org/11363163/diff/7001/tools/graphviz-create.py#n...
tools/graphviz-create.py:607: reportText = """<html>
On 2012/11/16 17:31:57, Jakob wrote:
> Why are these (constant) variables when Bold() etc. above were functions?

Good point. I got rid of the Bold() function, but I'd like to just keep this
html template as a big string for simplicity. The little functions to add
color/bold/etc., are for use when a function builds html from smaller elements.
This "reportText" variable might eventually go into another file, will things
like tooltips to explain the meaning of hydrogen instructions or things like
that.

https://codereview.chromium.org/11363163/diff/7001/tools/graphviz-create.py#n...
tools/graphviz-create.py:637: os.mkdir(dirname)
On 2012/11/16 17:31:57, Jakob wrote:
> You probably want os.makedirs() here, which can also deal with subdirectories.

Done.

https://codereview.chromium.org/11363163/diff/7001/tools/graphviz-create.py#n...
tools/graphviz-create.py:697: exit_code = 0
On 2012/11/16 17:31:57, Jakob wrote:
> Did you mean to overwrite this when some error occurs? I don't see any further
> assignments.

Done.

Powered by Google App Engine
This is Rietveld 408576698