[PATCH] [libcxx] Allow use of ShTest in libc++ tests along with other changes.
eric at efcs.ca
Thu Jan 22 08:33:47 PST 2015
Answer @danalbert's questions. Thanks for the review.
Comment at: test/libcxx/compiler.py:6
@@ -5,2 +5,3 @@
- def __init__(self, path, flags=, compile_flags=, link_flags=, use_ccache=False):
+ def __init__(self, path, flags=, compile_flags=, link_flags=,
> 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):
> 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):
> `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,
> 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.
> 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.
More information about the cfe-commits