silvas added a comment.
In http://reviews.llvm.org/D15462#309889, @beanz wrote:
> Sean,
>
> The reason for restricting to Unix is two fold. (1) the shell script goop,
> which I can replace with python and (2) I don't have a windows box to test
> on, so I didn't want people to think it worked.
>
> If I replace the shell goop with Python this will probably "Just Work"
> everywhere, so I can do that.
The lack of a windows box is totally understandable. Should Work is fine. I'll
probably give this a shot on windows some time and we'll catch anything then.
================
Comment at: utils/perf-training/perf-helper.py:14
@@ +13,3 @@
+
+def findProofrawFiles(path):
+ profraw_files = []
----------------
typo: Proofraw
================
Comment at: utils/perf-training/perf-helper.py:28
@@ +27,3 @@
+ os.remove(profraw)
+ return
+
----------------
The explicit return isn't needed if you aren't returning anything (but I think
you want to `return 0`).
================
Comment at: utils/perf-training/perf-helper.py:33
@@ +32,3 @@
+ print 'Usage: %s clean <llvm-profdata> <output> <path>\n\tMerges all
profraw files from path into output.' % __file__
+ return
+ cmd = [args[0], 'merge', '-o', args[1]]
----------------
You seem to be using the return value of this function (and `clean`) as though
they returned a value, but they don't seem to. Did you mean `return 1` or
whatever for these return's?
================
Comment at: utils/perf-training/perf-helper.py:36
@@ +35,3 @@
+ cmd.extend(findProofrawFiles(args[2]))
+ subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE)
+ return
----------------
I think you can use `subprocess.call` or `subprocess.check_call` here (they are
just sugar API around Popen, but they cover the common cases like this case).
================
Comment at: utils/perf-training/perf-helper.py:40
@@ +39,3 @@
+commands = ['clean', 'merge']
+return_code = 0
+
----------------
I don't think you need this variable. Everywhere you set it you could just do
`sys.exit(<the thing you were setting return_code to>)`
================
Comment at: utils/perf-training/perf-helper.py:44
@@ +43,3 @@
+ if len(sys.argv) >= 2 and sys.argv[1] in commands:
+ return_code = eval(sys.argv[1] + "(sys.argv[2:])")
+ elif len(sys.argv) < 2:
----------------
Cute eval, but probably better to just use an explicit dictionary. Personally,
I don't think we need the error handling (this script is never invoked by hand
in regular duty, I don't think).
Something like this:
```
def main():
COMMANDS = {'merge': merge, 'clean': clean}
f = COMMANDS[sys.argv[1]]
sys.exit(f(sys.argv[2:]))
```
That is one of the beauties (IMO) of python. The native safety of the language
and "sufficiently readable" behavior when an error occurs (e.g. sys.argv[1]
doesn't exist, invalid command is specified, etc.; you will get a stack trace
showing the line where the error occurred and a readable error message) means
that you can actually go quite far for scripts that are not meant to be
explicitly and frequently "human-invoked" as their primary purpose.
I don't think it's worth going overboard for a quick script like this.
http://reviews.llvm.org/D15462
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
