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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits