[PATCH] D59118: creduce script for clang crashes

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

(A few python style comments; feel free to ignore, and feel free to land no matter what you do with the comments.)



================
Comment at: clang/utils/creduce-clang-crash.py:1
+#!/usr/bin/env python
+# A tool that calls C-Reduce to create a minimal reproducer for clang crashes
----------------
rnk wrote:
> george.burgess.iv wrote:
> > nit: please specify a python version here, since the world is steadily making `python` == `python3` (if `pipes.quote` is working, I assume you'll want `#!/usr/bin/env python2`)
> Please don't do that, there is no python2.exe or python3.exe on Windows. `#!/usr/bin/env python` is the simplest thing that could work.
There's no python2 on mac either. `#!/usr/bin/env python` is the correct first line for executable python scripts.


================
Comment at: clang/utils/creduce-clang-crash.py:29
+        return exe_file
+  return None
+
----------------
There's `distutils.spawn.find_executable()` which does this for you.


================
Comment at: clang/utils/creduce-clang-crash.py:68
+def main():
+  parser = ArgumentParser(description='Runs C-Reduce on the input file')
+  parser.add_argument('build_script', type=str, nargs=1,
----------------
It's a somewhat common pattern to put a module docstring at the top of the file instead of a comment which explains usage, and then say `description=__doc__` here; see llvm/utils/gn/build/symlink_or_copy.py for an example.


================
Comment at: clang/utils/creduce-clang-crash.py:90
+    print(("ERROR: input file '%s' does not exist") % build_script)
+    sys.exit(1)
+
----------------
It's a bit more common to say `return 1` here and do `sys.exit(main())` at the bottom.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59118/new/

https://reviews.llvm.org/D59118