dlj added inline comments.
================
Comment at: clang/test/lit.cfg:23
# the test runner updated.
-config.test_format = lit.formats.ShTest(execute_external)
+config.test_format = lit.formats.ShTest(not llvm_config.use_lit_shell)
----------------
Minor nit: it seems reasonable enough to name this parameter:
ShTest(execute_external=(not llvm_config.use_lit_shell))
(I don't think that's a problem for ShTest.__init__, and has the benefit of
failing if the parameter name changes. Imagine if someone added another
default-valued bool argument to ShTest.__init__...)
================
Comment at: llvm/utils/lit/lit/llvm/config.py:93
def with_environment(self, variable, value, append_path = False):
- if append_path and variable in self.config.environment:
+ if append_path:
+ # For paths, we should be able to take a list of them and process
all
----------------
Looking at the callers of this... should the path-append logic be a separate
function?
For example:
llvm_config.with_environment('PATH', [config.llvm_tools_dir,
config.clang_tools_dir], append_path=True)
versus maybe this:
llvm_config.append_env_pathvar('PATH', [config.llvm_tools_dir,
config.clang_tools_dir])
================
Comment at: llvm/utils/lit/lit/llvm/config.py:128-129
+ def clear_environment(self, variables):
+ for name in variables:
+ if name in self.config.environment:
+ del self.config.environment[name]
----------------
You could also say:
```
for name in variables:
self.config.environment.pop(name, None)
```
================
Comment at: llvm/utils/lit/lit/llvm/config.py:132
+
+ def feature_config(self, features, encoding = 'ascii'):
+ # Ask llvm-config about the specified feature.
----------------
I think the expected format of the 'features' arg is complex enough that you
should add a docstring that explains it...
================
Comment at: llvm/utils/lit/lit/llvm/config.py:134
+ # Ask llvm-config about the specified feature.
+ arguments = [x for (x, _) in features]
try:
----------------
I think you could probably just use features.keys() for this, no?
================
Comment at: llvm/utils/lit/lit/llvm/config.py:147
+ output = output.decode(encoding)
+ lines = output.split('\n')
+ for (line, (_, patterns)) in zip(lines, features):
----------------
You might want to use splitlines() for Windows compatibility.
https://reviews.llvm.org/D37818
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
