Answer @danalbert's questions. Thanks for the review.
================ Comment at: test/libcxx/compiler.py:6 @@ -5,2 +5,3 @@ class CXXCompiler(object): - def __init__(self, path, flags=[], compile_flags=[], link_flags=[], use_ccache=False): + def __init__(self, path, flags=[], compile_flags=[], link_flags=[], + use_ccache=False): ---------------- danalbert wrote: > 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): ---------------- danalbert wrote: > 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): ---------------- danalbert wrote: > `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, ---------------- danalbert wrote: > 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. ---------------- danalbert wrote: > 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. http://reviews.llvm.org/D7073 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
