[PATCH] [libcxx] Allow use of ShTest in libc++ tests along with other changes.

[Eric Fiselier]

Address @danalbert's comments. Changes to come.


================
Comment at: test/libcxx/compiler.py:95
@@ -93,1 +94,3 @@
 
+    def _compileLinkTwoSteps(self, infile, object_file, out=None, flags=[],
+                            env=None, cwd=None):
----------------
danalbert wrote:
> Two args have different style: `infile` vs `object_file`. Prefer `in_file` (or even `source_file`). Same goes for the public API to this.
Will change.

================
Comment at: test/libcxx/compiler.py:102
@@ +101,3 @@
+        return self.link(object_file, out=out, flags=flags, env=env,
+                          cwd=cwd)
+
----------------
danalbert wrote:
> Alignment. Should run pylint and pep8 to catch these things.
I always find that pylint is too noisy. I'll try pep8.

================
Comment at: test/libcxx/test/config.py:179
@@ +178,3 @@
+        if use_lit_shell_default is not None:
+            use_lit_shell_default = (use_lit_shell_default != '0')
+        else:
----------------
danalbert wrote:
> Parentheses aren't necessary.
Will fix.

================
Comment at: test/libcxx/test/config.py:181
@@ +180,3 @@
+        else:
+            use_lit_shell_default = sys.platform in ['win32']
+        # Check for the command line parameter using the default value if it is
----------------
danalbert wrote:
> Why isn't this just a string comparison?
Yep. I just copied it from the clang lit.cfg but I'll change it in ours.

================
Comment at: test/libcxx/test/format.py:126
@@ +125,3 @@
+            # override this, cleanup is your reponsibility.
+            self._clean(object_path)
+            self._clean(exec_path)
----------------
danalbert wrote:
> This breaks the expectation of what `_clean` is used for. In the case of building on one machine but running on another, there's no way to determine which files need to be cleaned from which machine now. (I think in my case we're just ignoring any failures from clean failures, so there's no behavior change, but it's still odd). Why isn't `object_path` cleaning up after itself anymore?
> 
> If there is a good reason for `object_path` not cleaning up after itself, this is fine for now and I'll clean it up later if I think of a good way to do so.
I just cleaned it up using `_clean` because `_clean` does exactly what I need. I'll create another function that does the same thing as `_clean` and call that instead.

================
Comment at: test/libcxx/test/format.py:134
@@ +133,3 @@
+            contents = f.read()
+        use_verify = 'USE_VERIFY' in contents and self.use_verify_for_fail
+        extra_flags = []
----------------
danalbert wrote:
> Don't you want to check for `'// USE_VERIFY' in contents`?
> 
> This is slower than it used to be. We no longer stop processing after the first non-empty non-comment line, so this could take a long time on large tests.
> Don't you want to check for '// USE_VERIFY' in contents?

The old behavior only checked to see if it was in the line. This is definitely slower but .fail.cpp tests should be very small anyway so it shouldn't make too much of a difference. 

I would like to move this into parseIntegratedTestScript but this is the best way to currently do it.
I'll add a TODO.

================
Comment at: test/libcxx/test/format.py:135
@@ +134,3 @@
+        use_verify = 'USE_VERIFY' in contents and self.use_verify_for_fail
+        extra_flags = []
+        if use_verify:
----------------
danalbert wrote:
> `extra_flags = ['-Xclang', '-verify'] if use_verify else []` is fine for something this simple.
Will do.

================
Comment at: test/libcxx/test/format.py:138
@@ +137,3 @@
+            extra_flags += ['-Xclang', '-verify']
+        cmd, out, err, rc = self.cxx.compile(source_path, out='/dev/null',
+                                             flags=extra_flags)
----------------
danalbert wrote:
> Is there some sort of `os.null` that would work on Windows for this?
> 
> (not required for this patch, just thinking)
`os.devnull` should work. Thanks.

================
Comment at: utils/not/not.py:7
@@ +6,3 @@
+
+# not.py is a utility for inverting the return code of commands. It acts similar
+# to llvm/utils/not.
----------------
danalbert wrote:
> What's wrong with `// RUN: ! %rest_of_cmd`?
`!` is not available in the LIT internal shell. LLVM and clang use `llvm/utils/not` for this purpose. This is a simplified version that doesn't introduce a dependency on a bunch of LLVM libraries.

================
Comment at: utils/not/not.py:14
@@ +13,3 @@
+# NOTE: this is stolen from llvm/utils/lit/lit/util.py because python probably
+# won't be able to find the lit import.
+def which(command, paths = None):
----------------
danalbert wrote:
> Why not? We're already importing other things from `lit`. (Not going to actually review any of the rest of this file since I expect it's just copied directly.)
Because I have no idea how to find the location of the `lit` module during configuration and there aren't any easy ways to transfer the `lit` module location to `not.py`. 

If you have any ideas on how to find the location of the `lit` module I'll try and pass that information to `not.py`.

http://reviews.llvm.org/D7073

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/