[cfe-dev] Replacing the non-portable diff invocation coded as %diff_plist (and friends)

[Hubert Tong via cfe-dev]

On Mon, Jun 10, 2019 at 9:26 PM Reid Kleckner <rnk at google.com> wrote:

> The lines with paths looked like this:
>   <string>C:\src\llvm-project\clang\test\Analysis\unix-fns.c</string>
>
Thanks. Looks like just the CRLF problem then, the line format otherwise
matches one of the patterns that was present in the patch already ('
^[[:space:]]*<string>.:.*</string>$').

-- HT


>
> On Mon, Jun 10, 2019 at 6:20 PM Hubert Tong <
> hubert.reinterpretcast at gmail.com> wrote:
>
>> On Mon, Jun 10, 2019 at 7:29 PM Reid Kleckner <rnk at google.com> wrote:
>>
>>> I reverted this series of patches in r363007, since there were multiple
>>> issues with the new approach on Windows:
>>> - The heuristic for detecting path strings was incomplete, since it
>>> didn't account for drive letter style paths.
>>> - The -w option appeared to be necessary for ignoring CRLF.
>>> - The '</string>$' portion of the new pattern doesn't appear to match
>>> CRLF lines.
>>>
>>> This was enough that fixing forward was prohibitively difficult, so I
>>> reverted them.
>>>
>> Thanks. Would you mind sending me an example path string, and I'll look
>> into updating the path string detection. The `-b` option appears to take
>> care of CRLF, and I'll add '[[:space:]]*' before '$' to account for CRLF.
>>
>>
>>>
>>> On Tue, Apr 2, 2019 at 1:08 PM Hubert Tong via cfe-dev <
>>> cfe-dev at lists.llvm.org> wrote:
>>>
>>>> Various tests in `clang/test/Analysis/` invoke `diff` with a
>>>> non-portable `-I` option. I do not believe that a requirement for a `diff`
>>>> utility that supports such an extension is intended by the community. I
>>>> believe that replacing `diff -I` with `diff` on files that have been
>>>> normalized using `grep -v` is a reasonable solution. A minor detail is that
>>>> the output being checked is produced without a newline at the end of the
>>>> file. As part of the proposed solution, the file is rehabilitated as a text
>>>> file (as required by `grep`) by adding such a newline. The same is done for
>>>> the reference expected files that are missing a newline at the end of the
>>>> file. The solution is sketched out below for feedback.
>>>>
>>>> The normalization is in many cases encoded in a lit substitution
>>>> `%diff_plist`. A sample application of the proposed change would look like
>>>> the following. See further below for additional rationale on the
>>>> particulars.
>>>>
>>>> Replace the RUN line:
>>>> -// RUN: cat %t.plist | %diff_plist %S/Inputs/expected-plists/unix-fns.c.plist
>>>> -
>>>> +// RUN: %normalize_plist <%S/Inputs/expected-plists/unix-fns.c.plist
>>>> >%t.expected.sed.plist && echo >>%t.plist && %normalize_plist <%t.plist |
>>>> diff -u %t.expected.sed.plist -
>>>>
>>>> Replace the lit substitution:
>>>> -# Diff command used by Clang Analyzer tests (when comparing .plist
>>>> files
>>>> +# Filtering command used by Clang Analyzer tests (when comparing
>>>> .plist files
>>>>  # with reference output)
>>>> -config.substitutions.append(('%diff_plist',
>>>> -    'diff -u -w -I "<string>/" -I "<string>.:" -I "version"'))
>>>> +config.substitutions.append(('%normalize_plist',
>>>> +    "grep -Ev '%s|%s|%s'" %
>>>> +        ('^[[:space:]]*<string>.* version .*</string>$',
>>>> +         '^[[:space:]]*<string>/.*</string>$',
>>>> +         '^[[:space:]]*<string>.:.*</string>$')))
>>>>
>>>> Why not use more `RUN` lines?
>>>> The output being checked is line-number sensitive. Keeping the same
>>>> number of `RUN` lines minimizes unnecessary noise and risk of errors.
>>>>
>>>> Why not use `sed`?
>>>> `sed` can be used to normalize the lines that are expected to vary;
>>>> however, the some of the expected-output files have these lines omitted.
>>>> The `grep` replacement is more compatible with the behaviour of `diff
>>>> -I`. The filtered files are nevertheless named with "sed" for the
>>>> connotation that substitution took place.
>>>>
>>>> Why remove `-w` from `diff`?
>>>> It appears that the `-w` option was being used for its effect (in some
>>>> implementations) of ignoring the difference between a file that ends with a
>>>> newline and one which does not. Following normalization of both files to
>>>> end in a newline, this is no longer necessary. Since the effect is
>>>> non-portable, keeping the option (and thus allowing the non-portable effect
>>>> to occur, and perhaps become relied upon during development) is potentially
>>>> harmful.
>>>>
>>>> Why not add the newline via `cat`?
>>>> The newline can be added (in this context) using `echo | cat file -` as
>>>> opposed to using `echo >>file`; however, the `lit` implementation of `cat`
>>>> does not operate as expected on the standard input.
>>>> _______________________________________________
>>>> cfe-dev mailing list
>>>> cfe-dev at lists.llvm.org
>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20190610/ca860677/attachment.html>