When we talked about this you had mentioned that this would make it easier to access the tests that were built for debugging. I don't see anything here that does that. Is that going to be a follow up patch?
================ Comment at: test/libcxx/util.py:34 @@ +33,3 @@ +@contextmanager +def withFilename(name): + # yeilds a filename within a with statement. No action is taken upon scope ---------------- This is really a `NullContextManager` (nothing is specific to a file name). I'm not sure what a good name for the function would be. Possibly just `nullContext`? ================ Comment at: utils/not/not.py:3 @@ +2,3 @@ + +# not.py is a utility for inverting the return code of commands. +# It acts similar to llvm/utils/not. ---------------- The block should be the module docstring. ================ Comment at: utils/not/not.py:12 @@ +11,3 @@ + argv = sys.argv + del argv[0] + if len(argv) > 0 and argv[0] == '--crash': ---------------- `sys.argv` should be treated as constant, imo. Make a copy if you're going to use it this way. ================ Comment at: utils/not/not.py:20 @@ +19,3 @@ + return 1 + prog = lit.util.which(argv[0]) + if prog is None: ---------------- Why does LIT use its own home grown version of this? https://docs.python.org/2/distutils/apiref.html#module-distutils.spawn There's even a comment in the code that says "Would be nice if Python had a lib function for this". Was `distutils.spawn.find_executable` not available when that was written? If you use the `distutils` API (and just use subprocess instead of `lit.util.executeCommand` for this you can get rid of the need for `--lit-site`, which makes up most of the gross in this file. ================ Comment at: utils/not/not.py:25 @@ +24,3 @@ + out, err, rc = lit.util.executeCommand(argv) + if rc == 0 and not expectCrash: + return 1 ---------------- This doesn't directly mirror the one in `llvm/utils`. The other one only considers negative values success if passed `--crash`. Aside from that, it's kind of a weird flag imo. I would have made it a separate `expect_crash` command. `not --crash` reads to me as "doesn't crash". http://reviews.llvm.org/D7073 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
