[PATCH] D37778: [lit] Add a shared llvm configuration library, and use it in llvm

[David L. Jones via Phabricator via llvm-commits]

dlj added a comment.

Mostly just nit-picky Python comments... the helper class seems to reduce the overhead quite a bit.



================
Comment at: llvm/utils/lit/lit/llvm/config.py:6
+
+def _is_true_like(lit_config, value):
+    if value is None:
----------------
It might make sense to move this into the LLVMConfig class, so you don't have to keep passing lit_config.

It also looks like this is always called with attributes on config, so you could roll in the getattr()s here as well.


================
Comment at: llvm/utils/lit/lit/llvm/config.py:21
+
+class LLVMConfig:
+    def __init__(self, lit_config, config):
----------------
Nit: this should probably inherit from object for the benefit of py2.

class LLVMConfig(object):



================
Comment at: llvm/utils/lit/lit/llvm/config.py:39
+        # LIT_USE_INTERNAL_SHELL is in the environment, we use that as an override.
+        use_lit_shell = os.environ.get("LIT_USE_INTERNAL_SHELL")
+        if use_lit_shell:
----------------
(Minor suggestion) It might make sense to decide the platform default earlier, then pass it as the default to get(). That should let you collapse both of the next blocks:


```
shell_default = ('0' if sys.platform not in ['win32'] else '')
if os.environ.get('LIT_USE_INTERNAL_SHELL', shell_default) == '0':
  self.use_lit_shell = False
else:
  self.use_lit_shell = True
  config.available_features.add('shell')

```
(Not sure if that's much clearer or not, especially with the comments.)


================
Comment at: llvm/utils/lit/lit/llvm/config.py:59
+        # Sanitizers.
+        use_sanitizer = getattr(config, 'llvm_use_sanitizer', None)
+        if use_sanitizer:
----------------
If this value is set, will it be in CMake's internal semicolon-separated format? It might be better to split the value into a list if you can, instead of relying on substring searching.


================
Comment at: llvm/utils/lit/lit/llvm/config.py:60
+        use_sanitizer = getattr(config, 'llvm_use_sanitizer', None)
+        if use_sanitizer:
+            if 'Address' in use_sanitizer:
----------------
If you pass an iterable default to getattr() instead of None, you should be able to just omit this check.


================
Comment at: llvm/utils/lit/lit/llvm/config.py:61
+        if use_sanitizer:
+            if 'Address' in use_sanitizer:
+                config.available_features.add("asan")
----------------
You could cut down on the duplication here with a lambda:

_check = lambda name, feature: ('' if name in use_sanitizer else 'not_') + feature
config.available_features.add(_check('Address', 'asan'))


================
Comment at: llvm/utils/lit/lit/llvm/config.py:75
+        have_zlib = getattr(config, 'have_zlib', None)
+        if config.have_zlib:
+            config.available_features.add("zlib")
----------------
It looks like this should be have_zlib... but in any case, you could reduce duplication (and the potential for typops) by building the string directly with a ternary:

config.available_features.add(('' if getattr(config, 'have_zlib', None) else 'no') + 'zlib')


================
Comment at: llvm/utils/lit/lit/llvm/config.py:91
+
+        def should_use_gmalloc(lit_config):
+            # Check if we should use gmalloc.
----------------
(Side note: if you move _is_true_like into the class, you can probably just move the logic of this function into the if statement.)


================
Comment at: llvm/utils/lit/lit/llvm/config.py:109
+    def with_environment(self, variable, value, **kwargs):
+        append_path = kwargs.get('append_path', None)
+        if append_path == True and variable in self.config.environment:
----------------
It would probably make more sense for this to be a default-valued argument:

  def with_environment(..., append_path=False)

I don't see any reason that you would want to reprocess or pass along other kwargs, so the default valued option is probably clearer.


================
Comment at: llvm/utils/lit/lit/llvm/config.py:114
+
+            # Move it to the front if it already exists, otherwise insert it at the
+            # beginning.
----------------
Minor nit: maybe call the variable "prepend_path"?


================
Comment at: llvm/utils/lit/lit/llvm/config.py:118
+            current_value = self.config.environment[variable]
+            items = [norm(x) for x in current_value.split(os.path.pathsep)]
+            try:
----------------
Are you sure it's safe normalize the existing values?

It seems like it should be OK, since you won't be re-normalizing all the time. Doing normalized comparisons would be a bit more complex, too.


================
Comment at: llvm/utils/lit/lit/llvm/config.py:118
+            current_value = self.config.environment[variable]
+            items = [norm(x) for x in current_value.split(os.path.pathsep)]
+            try:
----------------
dlj wrote:
> Are you sure it's safe normalize the existing values?
> 
> It seems like it should be OK, since you won't be re-normalizing all the time. Doing normalized comparisons would be a bit more complex, too.
I think you want this to be:

os.path.pathsep.split(current_value)


================
Comment at: llvm/utils/lit/lit/llvm/config.py:119
+            items = [norm(x) for x in current_value.split(os.path.pathsep)]
+            try:
+                index = items.index(value)
----------------
You can narrow the scope here:

  try:
    items.remove(value)
  except ValueError:
    pass
  items = [value] + items


================
Comment at: llvm/utils/lit/lit/llvm/config.py:128
+
+    def with_system_environment(self, vars, **kwargs):
+        if vars is str:
----------------
Minor nit: 'vars' is a Python builtin name, maybe call it 'variables' instead?


================
Comment at: llvm/utils/lit/lit/llvm/config.py:129
+    def with_system_environment(self, vars, **kwargs):
+        if vars is str:
+            vars = [vars]
----------------
I think you want:

  if isinstance(vars, str):



================
Comment at: llvm/utils/lit/lit/llvm/config.py:144
+            print("Could not find llvm-config in " + self.config.llvm_tools_dir)
+            exit(42)
+
----------------
Should this use self.config.fatal?


================
Comment at: llvm/utils/lit/lit/llvm/config.py:146
+
+        if re.search(r'ON', llvm_config_cmd.stdout.read().decode('ascii')):
+            self.config.available_features.add(feature)
----------------
I think you might want to use this pattern instead:

  output, _ = llvm_config_cmd.communicate()
  if re.search('ON', output.decode('ascii')):
    # ...

That will handle the blocking semantics correctly on the subprocess's file handles. It also eliminates the need for the wait() below.


https://reviews.llvm.org/D37778