[PATCH] D37944: [lit] Move some of LLD and Clang's configuration code into LLVM shared configuration

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

dlj added inline comments.


================
Comment at: llvm/test/lit.cfg.py:153
+    ToolFilter('llvm-symbolizer', pre=JUNKCHARS),
+    ToolFilter('opt', JUNKCHARS),
+    ToolFilter('sancov', pre=JUNKCHARS),
----------------
pre=JUNKCHARS for consistency?


================
Comment at: llvm/utils/lit/lit/llvm/__init__.py:27
+
+            literal: If True, `name` is an exact regex that is passed to the
+            underlying substitution
----------------
(Minor nit) I really hate to bikeshed ... but I would expect this to do something like check for 'name' literally, without any regex matching. So I won't suggest changing the name to "is_regex" ;-) , but are there any other names that might make it more clear that the first argument will be interpreted as a well-formed regex?


================
Comment at: llvm/utils/lit/lit/llvm/__init__.py:30
+        """
+        def any_char_in(chars):
+            def escape_one(c):
----------------
I think re.escape will do what you need:
https://docs.python.org/2/library/re.html#re.escape


================
Comment at: llvm/utils/lit/lit/llvm/__init__.py:44
+            pre = any_char_in(pre)
+            regex = '(?<!(%s))' % pre
+            name = regex + name
----------------
It might make more sense to factor out the regex construction, then you can assign self.regex all at once:


```
def not_in(patterns, where=''):
  if not patterns:
    return ''
  pattern_str = '|'.join(re.escape(x) for x in patterns)
  return r'(?{where}!({pattern_str}))'.format(
      where=where, pattern_str=pattern_str)

self.regex = not_in(pre, where='<') + '\b' + name + '\b' + not_in(post)
```

(Not sure if that's actually clearer, but there might be some useful tidbits...)


================
Comment at: llvm/utils/lit/lit/llvm/config.py:174
+        # out its resource dir here in an easy to scrape form.
+        dir, _ = self.get_process_output([clang, '-print-file-name=include'], encoding='ascii')
+
----------------
Nit: 'dir' is a Python builtin, this shadows it.


https://reviews.llvm.org/D37944