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

[Zachary Turner via llvm-commits]

Thanks! Will fix
On Mon, Sep 18, 2017 at 10:20 PM David L. Jones via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170919/cd06a19f/attachment.html>