[PATCH] [libcxx] Allow use of ShTest in libc++ tests along with other changes.
danalbert at google.com
Wed Jan 21 14:00:23 PST 2015
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 @@
+ at contextmanager
+ # 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
+ if len(argv) > 0 and argv == '--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)
+ 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".
More information about the cfe-commits