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

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

jdenny added a comment.

In https://reviews.llvm.org/D47106#1150943, @probinson wrote:

> Regarding the DAG-NOT-DAG reordering diagnostic:  Rereading the comments, and the llvm-dev discussion, are we agreed that we can eliminate it?  That is, the first DAG group defines some match range, the second DAG group defines a disjoint match range, and the NOT group looks at the text from the end of the first match range to the start of the second match range.  No attempt to diagnose ordering problems.


Agreed.  I have a patch to implement that.  I'll clean it up and post it after we've cleared some of the existing queue.  It applies afterward.

> I also have some fussy comments about the torture test inline.

Thanks for the review.  I'm replying inline.



================
Comment at: test/FileCheck/check-dag-overlap-torture.txt:24
+; EndAfterEnd-NOT:  {{.}}
+; EndAfterEnd-SAME: ){{$}}
+
----------------
probinson wrote:
> The latest revision of the conceptual model says we won't allow SAME or NEXT after DAG.  For purposes of determining valid/invalid directive sequences, the NOT wouldn't count.  I think you still get the intended result without the SAME suffix.
> I see you're wanting to (in effect) have intra-line DAG, but DAG doesn't provide that guarantee.
> 
> The latest revision of the conceptual model says we won't allow SAME or NEXT after DAG.  For purposes of determining valid/invalid directive sequences, the NOT wouldn't count.  I think you still get the intended result without the SAME suffix.

Generally speaking, CHECK and CHECK-SAME always behave the same when the output is as expected for CHECK-SAME, right?  When the output is not as expected for CHECK-SAME, CHECK could match the same text on a later line, potentially delaying or eliminating the desired diagnostic.

> I see you're wanting to (in effect) have intra-line DAG, but DAG doesn't provide that guarantee.

Yes, that's what I want.  We don't have it, so I went for the next best thing.

Until we have an intra-line DAG, I'd like to request that we don't implement the restriction for SAME after DAG.

Even if we do implement an intra-line DAG, my gut says the SAME/NEXT after DAG restrictions eliminate reasonable use cases, but I don't have real-world examples right now, so I won't oppose the restrictions unless I come up with some examples.


================
Comment at: test/FileCheck/check-dag-overlap-torture.txt:36
+; EndAfterEnd-NOT:  {{.}}
+; EndAfterEnd-SAME: ){{$}}
+
----------------
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.


================
Comment at: test/FileCheck/check-dag-overlap-torture.txt:42
+; EndAfterEnd-NOT:  {{.}}
+; EndAfterEnd-SAME: ){{$}}
+
----------------
probinson wrote:
> This set of checks appears to be redundant with the previous `>xyz` set (the overlap is 3 characters instead of 1 but so what).
I'm just doing boundary testing.  None is a different class than one is a different class than multiple.  Why is one different than multiple?  Because off-by-one is a common bug that might cause incorrect behavior or mask another bug.

I don't see the advantage of removing this.


================
Comment at: test/FileCheck/check-dag-overlap-torture.txt:60
+; EndAfterEnd-NOT:  {{.}}
+; EndAfterEnd-SAME: ){{$}}
+
----------------
probinson wrote:
> This set of checks appears to be redundant with the previous `c<mno>xyz` set.
Again, I'm doing boundary testing, but on the match start: 44-48 (none past the start), 50-54 (one past the start), 56-60 (multiple past the start).




https://reviews.llvm.org/D47106