[PATCH] [LIT] Add SuffixDispatchTest for use in libc++ and libc++abi tests.
Eric Fiselier
eric at efcs.ca
Wed Nov 12 22:45:36 PST 2014
I've addressed most of @danalbert's comments. Patch to follow.
================
Comment at: utils/lit/lit/formats/__init__.py:3
@@ -2,1 +2,3 @@
from lit.formats.base import TestFormat, FileBasedTest, OneCommandPerFileTest
+from lit.formats.suffixdispatchtest import SuffixDispatchTest,\
+ CompileAndExecuteTestRunner
----------------
danalbert wrote:
> I much prefer
>
> from foo import (bar,
> baz)
>
> Trailing backslashes are gross.
Sounds good. I'll make the change.
================
Comment at: utils/lit/lit/formats/base.py:25
@@ -24,5 +24,3 @@
if not os.path.isdir(filepath):
- base,ext = os.path.splitext(filename)
- if ext in localConfig.suffixes:
- yield lit.Test.Test(testSuite, path_in_suite + (filename,),
- localConfig)
+ for ext in localConfig.suffixes:
+ if filepath.endswith(ext):
----------------
danalbert wrote:
> EricWF wrote:
> > This allows suffixes to have multiple extensions ex: `.pass.cpp`
> This looks odd to me, but phab isn't giving me any context...
>
> There's an outer loop here somewhere (judging by the yield below). I assume it's looping over each test?
>
> You then have `if filepath.endswith(ext)` within the for loop, making all but one iteration a no-op. Am I missing something here?
Nope, I think you've found a bug. I'll make a change.
================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:15
@@ +14,3 @@
+ SuffixDispatchTest dispatches tests based on their suffix to different
+ test runners. test runners must have function signature of
+ # __call__(self, test, lit_config).
----------------
danalbert wrote:
> 't'.upper()
Nice way to put that :)
================
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))]
----------------
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.
================
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:
> 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?
================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:147
@@ +146,3 @@
+ try:
+ compile_cmd = self.cxx.compileLinkCmd(['-o', exec_path, source_path])
+ cmd = compile_cmd
----------------
danalbert wrote:
> Why do we have to pass `-o` for this? Isn't it pretty obvious we want the link command to generate an output?
To make the interface simple. Then you have to worry about what parameter you pass to `compileLinkCmd` is the output file and which one is a set of extra arguments.
After playing around with it I decided the simplest interface is to just take an extra list of options.
================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:159
@@ +158,3 @@
+ cmd.append(exec_path)
+ if lit_config.useValgrind:
+ cmd = lit_config.valgrindArgs + cmd
----------------
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++).
================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:163
@@ +162,3 @@
+ cwd=source_dir)
+ report = """Compiled With: %s\n""" % \
+ ' '.join(["'%s'" % a for a in compile_cmd]) + report
----------------
danalbert wrote:
> Why a docstring?
No idea. Removed.
================
Comment at: utils/lit/lit/util.py:149
@@ +148,3 @@
+
+def _to_string(str_bytes):
+ if isinstance(str_bytes, str):
----------------
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.
================
Comment at: utils/lit/lit/util.py:189
@@ -165,1 +188,3 @@
+
+def executeCommandPTY(command, cwd=None, env=None):
try:
----------------
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.
================
Comment at: utils/lit/lit/util.py:206
@@ +205,3 @@
+ while True:
+ exitCode = p.poll()
+ if exitCode is not None:
----------------
danalbert wrote:
> `exit_code`
exitCode is the current naming in the rest of this file.
================
Comment at: utils/lit/lit/util.py:276
@@ +275,3 @@
+
+class Compiler(object):
+ def __init__(self, path, compile_flags=[], link_flags=[]):
----------------
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.
================
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:
> 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)
```
================
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:
> Isn't this implicit?
Don't think so. Python has shallow copy semantics on classes right?
================
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:
> 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.
http://reviews.llvm.org/D6206
More information about the llvm-commits
mailing list