[PATCH] D15462: [CMake] Add support for generating profdata for clang from training files

[Sean Silva via cfe-commits]

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