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

[Eric Fiselier]

================
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]):
----------------
danalbert wrote:
> danalbert wrote:
> > 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]`?
> > jroelofs: That assumes test suffixes have at most two '.'s in them...
> 
> No, it assumes it has at _least_ that many. Given `'foo.bar.baz.buzz.qux.cpp'`, this will still choose `qux`, which aiui is the desired behavior (and lets us move to a data structure that actually cleanly represents this data).
Because I wasn't committing to the suffix being the second part of the extension. I much prefer it matching suffixes is the simplest form.

================
Comment at: utils/lit/lit/util.py:189
@@ -165,1 +188,3 @@
+
+def executeCommandPTY(command, cwd=None, env=None):
     try:
----------------
danalbert wrote:
> 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`).
> What's wrong with -fcolorize=always?

This solution can be used with any program that by default will output color on a TTY. There was support for this change because FileCheck apparently outputs in color on TTY's as well.

Also GCC doesn't support it, so we would have to detect if the flag is supported.

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

I'll try that out, but I think the semantics are just different enough that it's not likely to work.




================
Comment at: utils/lit/lit/util.py:206
@@ +205,3 @@
+        while True:
+            exitCode = p.poll()
+            if exitCode is not None:
----------------
danalbert wrote:
> 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.
Ok.

================
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)
----------------
danalbert wrote:
> danalbert wrote:
> > 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)
> > jroelofs: Me too.
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)
```

I'll concede that my example is bad style, but copying lists into objects is the existing style in LIT.
Also I don't see how copying it would cause a problem but I do see how not copying it is a problem.

Is there a reason you want the compile_flags in the Compiler object to refer to the same list it was passed?
I don't see the use case for this.

How strongly do you feel about this change?


================
Comment at: utils/lit/lit/util.py:282
@@ +281,3 @@
+
+    def clone(self):
+        return Compiler(self.path, self.compile_flags, self.link_flags)
----------------
danalbert wrote:
> 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.
I'll make the change but I suspect this might be needed since you'll likely pass the same Compiler object to multiple test runners. Each of these test runners may want to modify it.

================
Comment at: utils/lit/lit/util.py:285
@@ +284,3 @@
+
+    def compileCmd(self, compile_flags=[]):
+        return [self.path, '-c'] + self.compile_flags + compile_flags
----------------
danalbert wrote:
> 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.
Alright, I'll make the change.

http://reviews.llvm.org/D6206