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

[Jonathan Roelofs]

On 11/13/14 12:13 PM, Dan Albert wrote:
> ================
> 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.
What benefit do we get from shoving everything into the constructors?
>
> ================
> 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]`?
That assumes test suffixes have at most two '.'s in them...
>
> ================
> 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...
C can have unicode in it, no?
>
> ================
> 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)
Me too.
>
> ================
> 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
>
>

-- 
Jon Roelofs
jonathan at codesourcery.com
CodeSourcery / Mentor Embedded