[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:24
+; EndAfterEnd-NOT:  {{.}}
+; EndAfterEnd-SAME: ){{$}}
+
----------------
probinson wrote:
> 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.
> 
> 
> 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.

Your formalized description is what gives me hope that we can describe these behaviors in a way that people will understand, but I agree it's better if it's just intuitive without studying documentation.

> I'm okay with leaving them in.

In that case, I say we let it be for now.  We can revisit after we finish this series of patches and after we consider an intra-line DAG.  By then, more concrete examples might arise.


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


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

It might not matter in one particular correct implementation, but it can matter in subtle ways as implementations evolve clever new techniques and break.

> The advantage to removing it is that it's one less case for a human to understand when reading the test.

I don't agree that removing the tests for the "multiple cases" is going to make this test file substantially easier to understand.  Once you understand the general sequence of tests here, understanding the difference between one and many seems like very little more to ask.  I'm happy to add comments if you feel that would help.

Obviously, there are no hard rules on how to design test cases effectively, so we write them based on our own experiences and on general guidelines about things like off-by-one.  In the past, I've spent a lot of time writing tests for parsers, and I've found that carefully testing boundary conditions in this way is important.

I feel pretty strongly about this, but I don't want to have to stall this patch indefinitely because we have different views on proper testing.  If you're adamant, I'll defer.  Alternatively, we could invite a third opinion to break the tie.  You can pick whomever.


https://reviews.llvm.org/D47106