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

[Dan Albert]

Definitely run pylint and pep8. Mostly good, though I do have some questions and nits.


================
Comment at: test/libcxx/compiler.py:1
@@ -1,2 +1,2 @@
 
 import lit.util
----------------
Blank line?

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

================
Comment at: test/libcxx/compiler.py:102
@@ +101,3 @@
+        return self.link(object_file, out=out, flags=flags, env=env,
+                          cwd=cwd)
+
----------------
Alignment. Should run pylint and pep8 to catch these things.

================
Comment at: test/libcxx/test/config.py:34
@@ +33,3 @@
+        # recursively load a config even if it tries.
+        # TODO: This is one hell of a hack. Fix it.
+        def prevent_reload_fn(*args, **kwargs):
----------------
No kidding. I thought we were already safe from this before...

================
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:
----------------
Parentheses aren't necessary.

================
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
----------------
Why isn't this just a string comparison?

================
Comment at: test/libcxx/test/config.py:492
@@ +491,3 @@
+        # Configure compiler substitions
+        sub.append( ('%cxx', self.cxx.path) )
+        # Configure flags substitutions
----------------
No extra spaces inside parens.

================
Comment at: test/libcxx/test/config.py:494
@@ +493,3 @@
+        # Configure flags substitutions
+        flags_str         = ' '.join(self.cxx.flags)
+        compile_flags_str = ' '.join(self.cxx.compile_flags)
----------------
Style nit: I'd kill the vertical alignment. PEP8 agrees (https://www.python.org/dev/peps/pep-0008/#pet-peeves)

================
Comment at: test/libcxx/test/config.py:498
@@ +497,3 @@
+        all_flags = '%s %s %s' % (flags_str, compile_flags_str, link_flags_str)
+        sub.append( ('%flags'        , flags_str        ) )
+        sub.append( ('%compile_flags', compile_flags_str) )
----------------
I'm not a fan of the vertical alignment here. If you want to keep things a little cleaner than many calls to `append`,

    sub.extend([
        ('foo', foo),
        ('bar', bar),
    ])

looks cleaner imo.

================
Comment at: test/libcxx/test/config.py:514
@@ +513,3 @@
+        exec_env_str = 'env ' if len(self.env) != 0 else ''
+        for k,v in self.env.items():
+          exec_env_str += ' %s=%s' % (k, v)
----------------
`k, v`

================
Comment at: test/libcxx/test/format.py:60
@@ +59,3 @@
+        res = lit.TestRunner.parseIntegratedTestScript(test,
+            require_script=is_sh_test)
+        # Check if a result for the test was returned. If so return that result.
----------------
Glad to see all the code backing this go away. Thanks!



================
Comment at: test/libcxx/test/format.py:126
@@ +125,3 @@
+            # override this, cleanup is your reponsibility.
+            self._clean(object_path)
+            self._clean(exec_path)
----------------
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.

================
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 = []
----------------
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.

================
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:
----------------
`extra_flags = ['-Xclang', '-verify'] if use_verify else []` is fine for something this simple.

================
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)
----------------
Is there some sort of `os.null` that would work on Windows for this?

(not required for this patch, just thinking)

================
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.
----------------
What's wrong with `// RUN: ! %rest_of_cmd`?

================
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):
----------------
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.)

http://reviews.llvm.org/D7073

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