arichardson added a comment. In D59725#1443990 <https://reviews.llvm.org/D59725#1443990>, @george.burgess.iv wrote:
> Only a few more nits on my side, and this LGTM. WDYT, arichardson? LGTM with the minor tempfile changes. ================ Comment at: clang/utils/creduce-clang-crash.py:201 + # Instead of modifying the filename in the test file, just run the command + empty_file = tempfile.NamedTemporaryFile() + is_interesting = self.check_expected_output(filename=empty_file.name) ---------------- `with tempfile.NamedTemporaryFile() as empty_file:`? Definitely works with python 3 and 2.7 docs say `This file-like object can be used in a with statement, just like a normal file.`. ================ Comment at: clang/utils/creduce-clang-crash.py:210 + # use delete=False in case the tmpfile flag causes problems when copying + tmpfile = tempfile.NamedTemporaryFile(delete=False) + ---------------- Use a with statement? ================ Comment at: clang/utils/creduce-clang-crash.py:212 + + cmd = self.get_crash_cmd() + ['-E', '-P'] + try: ---------------- Some crash messages might include the line numbers, do you think it makes sense to fall back to running with -E but without -P and also checking that? I do it in my script but I'm not sure preprocessing saves that much time since creduce will try to remove those statements early. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59725/new/ https://reviews.llvm.org/D59725 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
