<div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Apr 3, 2019 at 7:23 PM Artem Dergachev <<a href="mailto:noqnoqneo@gmail.com">noqnoqneo@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF">
Hi,<br>
<br>
Yup, sounds reasonable!<br>
<br>
Traditionally these tests were using FileCheck. The point of
replacing FileCheck with diff was that FileCheck failure reports for
incorrectly produced plist files on tests were very hard to
understand, especially when it was happening on a remote buildbot.
This was happening because plists are XML files and most bugs in
them usually look like large (10-20) lines suddenly appearing or
disappearing, with the most important information being in the
innermost XML tag in the middle of the chunk. Because FileCheck
reports only the first line that doesn't match, it made it very
annoying to debug. Diffs, on the other hand, are outright perfect
for the task.<br></div></blockquote><div>Thanks. The information on why FileCheck is not used here is helpful to know.</div><div><br></div><div>-- HT<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF">
<br>
An external normalization script doesn't anyhow contradict this
purpose, and if i understand correctly it was in fact one of the
possible solutions that was considered back in
<a class="gmail-m_5075482260053926093moz-txt-link-freetext" href="https://reviews.llvm.org/D50545#1195519" target="_blank">https://reviews.llvm.org/D50545#1195519</a><br>
<br>
<div class="gmail-m_5075482260053926093moz-cite-prefix">On 4/2/19 1:07 PM, Hubert Tong via
cfe-dev wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">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.<br>
</div>
<div dir="ltr"><br>
</div>
<div dir="ltr">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.<br>
<br>
Replace the RUN line:<br>
<font size="2"><span style="font-family:monospace,monospace">-//
RUN: cat %t.plist | %diff_plist %S/Inputs/expected-plists/</span></font><font size="2"><span style="font-family:monospace,monospace">unix-fns.c.plist
-<br>
+// RUN: %normalize_plist <%S/Inputs/expected-plists/</span></font><font size="2"><span style="font-family:monospace,monospace">unix-fns.c.plist
>%t.expected.sed.plist && echo >>%t.plist
&& %normalize_plist <%t.plist | diff -u
%t.expected.sed.plist -</span></font><br>
<br>
Replace the lit substitution:<br>
<span style="font-family:monospace,monospace">-# Diff command
used by Clang Analyzer tests (when comparing .plist files<br>
+# Filtering command used by Clang Analyzer tests (when
comparing .plist files<br>
# with reference output)<br>
-config.substitutions.append((</span><span style="font-family:monospace,monospace">'%diff_plist',<br>
- 'diff -u -w -I "<string>/" -I "<string>.:"
-I "version"'))<br>
+config.substitutions.append((</span><span style="font-family:monospace,monospace">'%normalize_plist',<br>
+ "grep -Ev '%s|%s|%s'" %<br>
+ ('^[[:space:]]*<string>.* version
.*</string>$',<br>
+ '^[[:space:]]*<string>/.*</</span><span style="font-family:monospace,monospace">string>$',<br>
+ '^[[:space:]]*<string>.:.*</</span><span style="font-family:monospace,monospace">string>$')))</span><br>
<br>
Why not use more `RUN` lines?<br>
The output being checked is line-number sensitive. Keeping the
same number of `RUN` lines minimizes unnecessary noise and
risk of errors.<br>
<br>
Why not use `sed`?<br>
`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.<br>
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.<br>
<br>
Why remove `-w` from `diff`?<br>
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.<br>
<br>
Why not add the newline via `cat`?<br>
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.<br>
</div>
</div>
<br>
<fieldset class="gmail-m_5075482260053926093mimeAttachmentHeader"></fieldset>
<pre class="gmail-m_5075482260053926093moz-quote-pre">_______________________________________________
cfe-dev mailing list
<a class="gmail-m_5075482260053926093moz-txt-link-abbreviated" href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>
<a class="gmail-m_5075482260053926093moz-txt-link-freetext" href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a>
</pre>
</blockquote>
<br>
</div>
</blockquote></div></div>