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

[Paul Robinson via Phabricator via llvm-commits]

probinson added inline comments.


================
Comment at: test/FileCheck/check-dag-overlap-torture.txt:24
+; EndAfterEnd-NOT:  {{.}}
+; EndAfterEnd-SAME: ){{$}}
+
----------------
jdenny wrote:
> 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.
Regarding SAME/NEXT after DAG, I do not have a strong opinion.  The question is mainly whether it will make sense to people who do not spend a lot of time studying how FileCheck works.  I'm okay with leaving them in.




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


================
Comment at: test/FileCheck/check-dag-overlap-torture.txt:42
+; EndAfterEnd-NOT:  {{.}}
+; EndAfterEnd-SAME: ){{$}}
+
----------------
jdenny wrote:
> 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.
In the case of string pattern matching I disagree that one is a different class than multiple.  I see it as a ternary `<=>`  result, and it doesn't matter how far unequal the substring boundaries are.
The advantage to removing it is that it's one less case for a human to understand when reading the test. 


https://reviews.llvm.org/D47106