<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, May 16, 2018 at 11:40 AM,  <span dir="ltr"><<a href="mailto:paul.robinson@sony.com" target="_blank">paul.robinson@sony.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I just stumbled across r332416, which was modifying a test with multiple<br>
identical CHECK-DAG directives, which reminded me I needed to get back<br>
to you about this... sorry for the slow response.<br></blockquote><div><br></div><div>Not a problem.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> From: Joel E. Denny [mailto:<a href="mailto:jdenny.ornl@gmail.com" target="_blank">jdenny.ornl@gmail.com</a>]<br>
<span>>> On Mon, May 7, 2018 at 12:56 PM, <<a href="mailto:paul.robinson@sony.com" target="_blank">paul.robinson@sony.com</a>> wrote:<br>
>>> 1. In a CHECK-DAG group, don't let the matches for patterns overlap.<br>
>>> 2. Add a new CHECK-DAG-N directive, where N is some integer, to express<br>
>>> that a pattern must have N non-overlapping matches.<br>
>><br>
>> I think #1 is much more intuitive and easy to describe/document than #2.<br>
>> Changing the meaning of DAG in that way is highly unlikely to affect any <br>
>> existing test, IMO.  And if it does, my first expectation is that the test <br>
>> wasn't doing what the author intended anyway.<br>
><br>
> I implemented #1, and I now have 105 new unexpected tests failures from<br>
> llvm core (58), clang (45), openmp runtime (1), and compiler-rt (1) test<br>
> suites.<br>
<br>
</span>Okay... obviously more than I expected.  But hey, if we look at it as a<br>
percentage of all tests, it's really small!  [attempting to salvage a<br>
degree of self-respect]<br></blockquote></div><div class="gmail_quote"><br></div><div class="gmail_quote">I didn't exactly expect it either.  :-)</div><div class="gmail_quote"></div><div class="gmail_quote"><div><span></span><br><span></span></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>
> I've only looked at a few failures so far.  Of those, the problem is<br>
> the expected one: in a CHECK-DAG group, multiple patterns match the<br>
> same line, so not permitting overlapping matches causes some patterns<br>
> never to match successfully.  However, all the overlapping matches so<br>
> far seem unintentional.  That is, this new implementation seems to be<br>
> catching test errors.<br>
<br>
</span>Awesome.  Catching test errors is a definite plus for the project.<br></blockquote><div><br></div><div>Thanks for encouraging me to pursue this implementation.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> ...<br>
<span>> Path Forward<br>
> ------------------<br>
><br>
> These examples seem like strong evidence that permitting overlapping<br>
> matches is a significant source of test errors.  Moreover, we have my<br>
> original goal that some test authors were apparently assuming was<br>
> already supported: to be able to handle sets of strings that are both<br>
> unordered and non-unique.  Nevertheless, it's possible that there are<br>
> use cases, such as the one suggested by S1, that we lose.<br>
<br>
</span>Hmmm... Either that one intended for the matches to be different, and<br>
it's a bug that they aren't; or it's an overly flexible test, and <br>
tightening it up does no harm.  The test change to accommodate the<br>
updated FileCheck behavior would have to go through the original test<br>
author or some other domain expert in order to determine which it is.<br>
<br>
I don't see the loss of flexibility as necessarily a bad thing; although<br>
we might find cases where behavior reasonably varies across targets, and<br>
then we would need to work out how to accommodate those differences.<br></blockquote><div><br></div><div>Agreed.</div><div><br></div><div>Thanks for the feedback.<br></div><div><br></div><div>Joel<br></div><div> <span></span><br><span></span></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>
> Here are some potential paths forward:<br>
><br>
> P1. Give up on approach #1 (non-overlapping CHECK-DAG) and go back to<br>
> #2 (CHECK-DAG-N).  I don't prefer this, but it would be less work.<br>
><br>
> P2. Make CHECK-DAG non-overlapping but fix all test cases before<br>
> committing that change.  Patch reviews will surely be slow.<br>
><br>
> P3. Make non-overlapping CHECK-DAG a special mode that's off by default<br>
> until all test suites have been fixed.<br>
<br>
</span>I vote for P3.  It's easier to divide up the work if the feature is in<br>
upstream FileCheck, and it's easy for someone doing the work to enable<br>
the feature in their local branch.  I would not bother making it a<br>
command-line switch, unless we really do come up with cases where the<br>
overlapping matches are absolutely the right solution for some class of<br>
tests.<br>
<br>
I suggest you file a bug with the complete list of test failures that<br>
non-overlapping CHECK-DAG finds.  That will help keep track of the work,<br>
and if anyone else feels moved to pick up some part of it, they can make<br>
notes in the bug so we can try to avoid duplicating effort.  Hopefully<br>
I can devote some time to this myself, I think it's how CHECK-DAG should<br>
have been done in the first place and I'm sad we didn't catch it at the<br>
time.<br>
<span><br>
> By the way, I think it would be generally useful for FileCheck to have<br>
> a debugging mode that prints every match.  It would be particularly<br>
> useful while migrating to a non-overlapping CHECK-DAG.<br>
><br>
> Thoughts?<br>
><br>
> Joel<br>
<br>
</span>A debugging mode like that sounds like a useful feature for writing a<br>
test, independent of the migration aid.  Do it.<br>
 <br>
--paulr<br>
<br>
</blockquote></div><br></div></div>