[PATCH] [LIT] Add SuffixDispatchTest for use in libc++ and libc++abi tests.

[Dan Albert]

================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:22
@@ +21,3 @@
+
+    def add_suffix_test(self, suffix, test_runner):
+        self.suffix_tests += [tuple((suffix, test_runner))]
----------------
EricWF wrote:
> danalbert wrote:
> > Is there ever a case where these aren't known at initialization time of the object? I'd vote for just removing these two methods (especially delete).
> Perhaps. I need to look into this more but the use case I envisioned was that sub directories may want to add/remove test suffixes.
Hmm. I'm guessing YAGNI. Even if some subdirectory has different needs (sounds unlikely), they can just add it at the top level.

================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:106
@@ +105,3 @@
+        test_runner = None
+        for suffix_test in self.suffix_tests:
+            if name.endswith(suffix_test[0]):
----------------
EricWF wrote:
> danalbert wrote:
> > EricWF wrote:
> > > jroelofs wrote:
> > > > I think you can do this in python....
> > > > 
> > > >   for (suffix, test) in self.suffix_tests:
> > > >     if name.endswith(suffix):
> > > >       test_runner = test
> > > >       break
> > > Cool! That's so much cleaner. Thanks.
> > You wanted a dict.
> > 
> >     test_runners = {
> >         ".pass": PassRunner(),
> >         ".fail": FailRunner()
> >     }
> > 
> > This block would become `test_runner = test_runners[suffix]`.
> I disagree. The problem is that I don't know how to split the filename into name + suffix. For example a `test.fail.cpp`. How do you know where to split the test name?
What's wrong with `testname.split('.')[-2]`?

================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:159
@@ +158,3 @@
+            cmd.append(exec_path)
+            if lit_config.useValgrind:
+                cmd = lit_config.valgrindArgs + cmd
----------------
EricWF wrote:
> danalbert wrote:
> > This looks like an implementation detail that leaked (though I appreciate the irony).
> > 
> > I'd like to see if we can factor this out some how... but I'm not coming up with anything right now.
> I'm more alright having this here as opposed to in libc++ and libc++abi's lit.cfg. The valgrind stuff is all defined by lit (and is not local to libc++).
Ah, didn't realize it was a common LIT thing. Works for me.

================
Comment at: utils/lit/lit/util.py:149
@@ +148,3 @@
+
+def _to_string(str_bytes):
+    if isinstance(str_bytes, str):
----------------
EricWF wrote:
> danalbert wrote:
> > jroelofs wrote:
> > > Some comments on these two functions would be nice here to explain the "why" behind them. At the moment I don't understand why these are needed.
> > Yeah, I find it really difficult to believe that a python library needs its own `to_string`.
> Not sure. These already existed in the utility library. I'm just trying not to break them :S. I'll inquire as to why they are needed.
Well, looking at it, it's for handling transformation of a unicode byte-string into a text string... but what I don't understand is why we'd ever have to deal with that input...

================
Comment at: utils/lit/lit/util.py:189
@@ -165,1 +188,3 @@
+
+def executeCommandPTY(command, cwd=None, env=None):
     try:
----------------
EricWF wrote:
> danalbert wrote:
> > Why do we need this? What's wrong with `subprocess.Popen(..., stdout=subprocess.PIPE, stderr=subprocess.PIPE)`?
> Color output! By tricking the sub process into thinking it's running in a TTY it will produce color output. I HATE that we currently lose clang pretty formatting when it gets passed through LIT.
Oh, yeah, that's rather nice. The implementation is rather disgusting though :) (I blame `subprocess.Popen`, not you).

What's wrong with `-fcolorize=always`?

If `-fcolorize=always` won't work, I think it would be better to make a `PtyPipe` class and pass that to executeCommand (with the default arugments being `subprocess.PIPE`).

================
Comment at: utils/lit/lit/util.py:206
@@ +205,3 @@
+        while True:
+            exitCode = p.poll()
+            if exitCode is not None:
----------------
EricWF wrote:
> danalbert wrote:
> > `exit_code`
> exitCode is the current naming in the rest of this file. 
`out_pty` and `err_pty` are the other style though. Let's be consistent throughout the file, whatever that may be.

================
Comment at: utils/lit/lit/util.py:276
@@ +275,3 @@
+
+class Compiler(object):
+    def __init__(self, path, compile_flags=[], link_flags=[]):
----------------
EricWF wrote:
> danalbert wrote:
> > `lit.util` doesn't seem like the right place for this (but I haven't had a look around to know what the right place would be).
> Your probably right but IDK where to put it either. It's a surprising useful abstraction.
We can keep it here for now if no one has a better home for it,

================
Comment at: utils/lit/lit/util.py:279
@@ +278,3 @@
+        self.path = str(path)
+        self.compile_flags = list(compile_flags)
+        self.link_flags = list(link_flags)
----------------
EricWF wrote:
> danalbert wrote:
> > The copy probably isn't necessary.
> I disagree. I already have a use-case for it in the patch that makes libc++ use this.
> 
> ```
> compile_flags = [...]
> cxx = Compiler(compile_flags)
> compile_flags.extend(sanitizer_args)
> sanitized_cxx = Compiler(compile_flags)
> ```
This might be the functional programmer in me showing, but I would write that as

    compile_flags = [...]
    cxx = Compiler(compile_flags)
    sanitized_cxx = Compiler(compile_flags + sanitizer_args)

================
Comment at: utils/lit/lit/util.py:282
@@ +281,3 @@
+
+    def clone(self):
+        return Compiler(self.path, self.compile_flags, self.link_flags)
----------------
EricWF wrote:
> danalbert wrote:
> > Isn't this implicit?
> Don't think so. Python has shallow copy semantics on classes right?
I suppose this depends on the resolution of the comments on 279. I think a shallow copy should be sufficient.

================
Comment at: utils/lit/lit/util.py:285
@@ +284,3 @@
+
+    def compileCmd(self, compile_flags=[]):
+        return [self.path, '-c'] + self.compile_flags + compile_flags
----------------
EricWF wrote:
> danalbert wrote:
> > For the way this is used, I think `def compilerCmd(self, in, out)` would make more sense.
> > 
> > Same goes for the rest of these.
> I still want to provide extra compile and link flags. meaning that the interface would look like:
> ```
> def compileCmd(self, in, out=None, compile_flags=[])
> ```
> Now you have to name the extra flags if you want to pass them. Instead I really thinks it's cleaner to let the user just pass arbitrary flags. 
> If you feel strongly I'll make the change, but I had originally implemented it as you suggested and I really didn't like it.
The above interface looks right to me.

Keep in mind that hard coding the `-o` at the call site means that we'll have implementation details of the compiler leaking out of the abstraction and each call site will be

    if compiler_under_test_is_msvc:
        compiler.compileCmd(whatever_msvc_needs)
    else:
        compiler.compileCmd(['-o', out_file, in_file])

The `Compiler` should be an abstraction. How the compiler (the actual compiler, not `Compiler`) works should be irrelevant from the outside.

http://reviews.llvm.org/D6206