[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 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/bc2124f1/attachment.html>