[llvm-commits] [LLVMdev] PR 8199 - Can we get patches reviewed?

[Jason Kim]

Hi David.

Feedback below:

On Tue, Dec 14, 2010 at 1:41 PM, David A. Greene <greened at obbligato.org> wrote:
> Jason Kim <jasonwkim at google.com> writes:
>
>> I think the regulars want the patches posted on llvm-commits for review
>> However, taking a  look at the second one (lit2.patch, attached),
>> there's couple of utilities missing in the list - like elf-dump etc.
>
> Ping?  Patch #3 on the bug is probably the one to apply.  I've copied it below.
>
>                                  -Dave
>
> Index: test/lit.cfg
> ===================================================================
> --- test/lit.cfg        (revision 121488)
> +++ test/lit.cfg        (working copy)
> @@ -148,6 +148,35 @@
>     else:
>         config.substitutions.append(('%' + sub, site_exp[sub]))
>
> +# NOTE: Poor man's regexp here.  Only ^ and $ are supported.
> +
> +# Replace command at the start of the line.
> +for sub in ['^llc', '^lli', '^tblgen', '^bugpoint', '^llvmc',
> +            '^llvm-ar', '^llvm-as', '^llvm-bcanalyzer', '^llvm-dis',
> +            '^llvm-extract', '^llvm-ld', '^llvm-link', '^llvm-nm',
> +            '^llvm-prof', '^llvm-ranlib', '^llvm-mc', '^opt',
> +            '^FileCheck']:
> +    config.substitutions.append((sub,
> +                                 llvm_tools_dir + '/' + sub[1:]))
> +

1. Please have one canonical list of executables (including things
like (elf/macho/coff)-dump that are used by test/ and prepend the
exact path to all of them, as having 3 sets of basically the same data
is not likely to be well received. Can you perhaps use word boundary
as a pattern check? (if you have to import re, so be it.)
2. Add verbose comment to lit.cfg and TestRunner.py
3. resend patch to llvm-commits after doublechecking test results.
4. COMMIT!
5. File a doc bug to remind the core developers about this change (or
patch the doc directly?) You can do this separately.

Steps 1,2, and 5 are to hopefully answer any objections that might
arise - as I can't say for certain why the llvm-gcc path was prepended
during config... (Tho I agree its a  bug!)

Thanks!

-jason

> +# Replace command at the end of the line.
> +for sub in [' llc$', ' lli$', ' tblgen$', ' bugpoint$', ' llvmc$',
> +            ' llvm-ar$', ' llvm-as$', ' llvm-bcanalyzer$', ' llvm-dis$',
> +            ' llvm-extract$', ' llvm-ld$', ' llvm-link$', ' llvm-nm$',
> +            ' llvm-prof$', ' llvm-ranlib$', ' llvm-mc$', ' opt$',
> +            ' FileCheck$']:
> +    config.substitutions.append((sub,
> +                                 ' ' + llvm_tools_dir + '/' + sub[1:-1]))
> +
> +# Replace command in the middle of the pipeline.
> +for sub in [' llc ', ' lli ', ' tblgen ', ' bugpoint ', ' llvmc ',
> +            ' llvm-ar ', ' llvm-as ', ' llvm-bcanalyzer ', ' llvm-dis ',
> +            ' llvm-extract ', ' llvm-ld ', ' llvm-link ', ' llvm-nm ',
> +            ' llvm-prof ', ' llvm-ranlib ', ' llvm-mc ', ' opt ',
> +            ' FileCheck ']:
> +    config.substitutions.append((sub,
> +                                 ' ' + llvm_tools_dir + '/' + sub[1:-1] + ' '))
> +
>  excludes = []
>
>  # Provide target_triple for use in XFAIL and XTARGET.
> Index: utils/lit/lit/TestRunner.py
> ===================================================================
> --- utils/lit/lit/TestRunner.py (revision 121488)
> +++ utils/lit/lit/TestRunner.py (working copy)
> @@ -8,6 +8,10 @@
>  import platform
>  import tempfile
>
> +# Utility to replace a string starting from the end.
> +def rreplace(s, old, new, count):
> +    return (s[::-1].replace(old[::-1], new[::-1], count))[::-1]
> +
>  class InternalShellError(Exception):
>     def __init__(self, command, message):
>         self.command = command
> @@ -448,7 +452,18 @@
>     def processLine(ln):
>         # Apply substitutions
>         for a,b in substitutions:
> -            ln = ln.replace(a,b)
> +            # Poor man's regexp: support ^ and $.
> +            if a.startswith('^') and a.endswith('$'):
> +                if ln == a[1:-1]:
> +                    ln = b
> +            elif a.startswith('^'):
> +                if ln.startswith(a[1:]):
> +                    ln.replace(a[1:],b,1)
> +            elif a.endswith('$'):
> +                if ln.endswith(a[:-1]):
> +                    ln = rreplace(ln,a[:-1],b,1)
> +            else:
> +                ln = ln.replace(a,b)
>
>         # Strip the trailing newline and any extra whitespace.
>         return ln.strip()
>