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

[Eric Fiselier]

Answer @danalbert's questions. Thanks for the review.


================
Comment at: test/libcxx/compiler.py:6
@@ -5,2 +5,3 @@
 class CXXCompiler(object):
-    def __init__(self, path, flags=[], compile_flags=[], link_flags=[], use_ccache=False):
+    def __init__(self, path, flags=[], compile_flags=[], link_flags=[],
+                 use_ccache=False):
----------------
danalbert wrote:
> pylint really hates [] as a default argument (and apparently for [[ http://stackoverflow.com/a/9526480/632035 | good reason ]]).
> 
> Making the defaults be `None` and using `self.flags = flags or []` looks nice.
> 
> (I understand that using `self.flags = list(flags)` actually works around this problem, but I'd like it if we could change the style so it cuts down on noise for those of us that do run linters.)
Huh, I had no idea default arguments worked like that in python.

I still think you should copy the list in so you would need to write `self.flags = list(flags or [])`. Which is more complicated that just `self.flags = list(flags)`. I'll make the change to reduce noise but I'm not convinced it is a good one.



================
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):
----------------
danalbert wrote:
> No kidding. I thought we were already safe from this before...
Two reasons for this change.

1. In order to turn this into a reusable function that function should probably return. Recursive reloading would prevent this.
2. I actually have plans to introduce a second benchmarking test suite that relies on the same site config file but it can't load the current lit.cfg

================
Comment at: test/libcxx/test/format.py:30
@@ +29,3 @@
+    # TODO: Move this into lit's FileBasedTest
+    def getTestsInDirectory(self, testSuite, path_in_suite,
+                            litConfig, localConfig):
----------------
danalbert wrote:
> `self` and `litConfig` are unused (this could be a function rather than a method). Is this "overriding" something from `lit.formats.FileBasedTest`?
It's replacing the method by the same name in `lit.formats.FileBasedTest` with a small tweak that allows multipart suffixes. I didn't want to make the change in LIT proper because (as previously discussed) we might try and move away from metadata in test names. 

================
Comment at: test/libcxx/test/format.py:76
@@ +75,3 @@
+        if is_sh_test:
+            return lit.TestRunner._runShTest(test, lit_config,
+                                             self.execute_external, script,
----------------
danalbert wrote:
> This is "private" to `lit.TestRunner`. Should this function be given a public name in `lit.TestRunner`? Is there some other API that should be used?
Maybe? I split `_runShTest()`  from `lit.TestRunner.executeShTest()` specifically for use with libc++. I'm hesitant to make the function public if we are the only people that need it. There is no other API in `lit.TestRunner` that would serve our needs.

================
Comment at: utils/not/not.py:5
@@ +4,3 @@
+
+"""not.py is a utility for inverting the return code of commands.
+It acts similar to llvm/utils/not.
----------------
danalbert wrote:
> Module docstrings go at the very top, before imports.
> 
> Whole file looks much better now, btw. Thanks. I'd still like it if this wasn't necessary, so we should maybe look at adding `!` to the builtin shell at some point, but this is plenty fine for now.
I've looked at the internal shell a bit and it seems that it could use a lot of love. I'll look into adding `!` but it is super low on my list.

http://reviews.llvm.org/D7073

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