[PATCH] D37818: [lit] Update clang and lld to use the new shared LLVMConfig stuff

David L. Jones via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Sep 13 12:40:14 PDT 2017


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





More information about the llvm-commits mailing list