[PATCH] D47106: [FileCheck] Make CHECK-DAG non-overlapping

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added inline comments.


================
Comment at: test/FileCheck/check-dag-overlap-torture.txt:36
+; EndAfterEnd-NOT:  {{.}}
+; EndAfterEnd-SAME: ){{$}}
+
----------------
probinson wrote:
> jdenny wrote:
> > probinson wrote:
> > > jdenny wrote:
> > > > probinson wrote:
> > > > > This is the first set of checks where there's a difference in behavior depending on overlap or not.  Given the test is trying to exercise exactly that, the first two sets of checks aren't useful.  (They might be appropriate for a general DAG functionality test, but not here.)
> > > > I intended for this test file to exhaustively exercise overlap detection.  That includes checking for both false positives and false negatives.
> > > > 
> > > > I see no reason to eliminate checking for false positives just because those cases behaved the same when we permitted overlaps.  I want to check that overlap detection works correctly in all cases now.
> > > Do you anticipate deleting this test when the overlap feature is removed?  If so, I won't argue about its content (much).
> > No, I primarily mean for this test to exercise overlap detection for DAG, which is important as long as non-overlapping is enforced.
> That's fine (clearly we'd remove the RUN lines with the deprecated switch, when that gets removed).
> But in that case, there are already a bunch of tests for non-overlapping DAG lines in other check-dag*.txt tests, and it distracts from the focus of this test to repeat them here. That would be the first two sets of checks in EndAfterEnd, and corresponding sets in the other cases.
> (Or if you think the existing DAG tests in the other files are inadequate, beef up those tests, and keep this one focused on overlap.) 
> That's fine (clearly we'd remove the RUN lines with the deprecated switch, when that gets removed).

Right.

> But in that case, there are already a bunch of tests for non-overlapping DAG lines in other check-dag*.txt tests, and it distracts from the focus of this test to repeat them here. That would be the first two sets of checks in EndAfterEnd, and corresponding sets in the other cases.
> (Or if you think the existing DAG tests in the other files are inadequate, beef up those tests, and keep this one focused on overlap.)

I see check-dag-overlap-torture.txt as a careful, organized sweep through all the various cases basic overlap detection must handle.  In my opinion, dropping chunks of it that happen to be covered for some other purpose elsewhere makes this test more confusing, and it makes the overall testing of overlap detection less maintainable.  I don't think those chunks distract from the focus of this test because the focus of this test is the correct behavior of the overlapping detection mechanism, and that includes no false positives, especially at boundary conditions.

As in the discussion of one vs. multiple, I'm pretty adamant, and I don't feel like we're converging.  Just give me the word, and I'll yield, or you can invite a third opinion if you know someone who might offer another perspective.

I'm not trying to be difficult on these points.  I just disagree.  But I'd still like us to find a path forward.  A third person might help.






https://reviews.llvm.org/D47106