<div dir="ltr">On Wed, May 16, 2018 at 12:38 PM, Joel E. Denny <span dir="ltr"><<a href="mailto:jdenny.ornl@gmail.com" target="_blank">jdenny.ornl@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><div class="gmail-h5"><div class="gmail_extra"><div class="gmail_quote">On Wed, May 16, 2018 at 12:24 PM,  <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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-m_-2598824098957802095HOEnZb"><div class="gmail-m_-2598824098957802095h5"><br>
<br>
> -----Original Message-----<br>
> From: Justin Bogner [mailto:<a href="mailto:justin@justinbogner.com" target="_blank">justin@justinbogner.co<wbr>m</a>] On Behalf Of Justin<br>
> Bogner<br>
> Sent: Wednesday, May 16, 2018 12:16 PM<br>
> To: llvm-dev<br>
> Cc: <a href="mailto:jdenny.ornl@gmail.com" target="_blank">jdenny.ornl@gmail.com</a>; Robinson, Paul<br>
> Subject: Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple<br>
> occurrences of string<br>
> <br>
> Paul Robinson <<a href="mailto:paul.robinson@sony.com" target="_blank">paul.robinson@sony.com</a>> writes:<br>
> >> From: Joel E. Denny [mailto:<a href="mailto:jdenny.ornl@gmail.com" target="_blank">jdenny.ornl@gmail.com</a>]<br>
> >>> 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<br>
> 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<br>
> #2.<br>
> >>> Changing the meaning of DAG in that way is highly unlikely to affect<br>
> any<br>
> >>> existing test, IMO.  And if it does, my first expectation is that the<br>
> 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)<br>
> test<br>
> >> suites.<br>
> ...<br>
> >> 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>
> > 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 feel like a variant of this where this mode is on by default would<br>
> make more sense. That is,<br>
> <br>
> 1. Modify CHECK-DAG so that overlapping patterns aren't allow, unless<br>
>    --deprecated-allow-overlapping<wbr>-check-dag or something is passed to<br>
>    FileCheck.<br>
> 2. Add the flag to the 105 current test failures<br>
> 3. File bugs and/or create reviews to remove the flag from each of the<br>
>    tests.<br>
> <br>
> There are two reasons I prefer this approach. First, new tests are<br>
> "correct by default" while we clean up the existing problematic tests.<br>
> Second, out of tree work has an easy path forward and reasonable<br>
> forewarning. They can add the flag temporarily and fix problematic tests<br>
> when time permits, rather it suddenly breaking their CI whenever open<br>
> source flips the switch.<br>
<br>
</div></div>Those are excellent points, and that approach totally makes sense.<br>
--paulr<br>
<br>
</blockquote></div></div><div class="gmail_extra"><br></div></div></div><div class="gmail_extra">Make sense.  I'll work on putting the patch in phabricator and creating the bugzilla issue.  I'll get to implementing the debugging mode eventually.</div></div></blockquote><div><br></div><div>Sorry for the delay.  It's in phabricator here:</div><div><br></div><div><a href="https://reviews.llvm.org/D47106">https://reviews.llvm.org/D47106</a></div><div><br></div><div>The list of test failures has changed slightly.  I'm re-running and will create a bugzilla soon.</div><div><br></div><div>Thanks.</div><div><br></div><div>Joel<br></div></div><br></div></div>