[PATCH] D47106: [FileCheck] Make CHECK-DAG non-overlapping
Paul Robinson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 9 12:37:00 PDT 2018
probinson accepted this revision.
probinson added a comment.
This revision is now accepted and ready to land.
Given that you feel so very strongly about it, and that this is a tool that really has absolutely zero exposure outside our own test system, I'm willing to say that erring on the side of extra testing is acceptable here.
================
Comment at: test/FileCheck/check-dag-overlap-torture.txt:42
+; EndAfterEnd-NOT: {{.}}
+; EndAfterEnd-SAME: ){{$}}
+
----------------
jdenny wrote:
> jdenny wrote:
> > 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.
> To be clear, when I say "I'll yield" or "I'll defer", I mean that I'm willing to do as you ask in order to proceed. Of course, I'd rather us reach agreement on what's right, but it doesn't seem like we're making arguments that are persuasive to each other, so I suggested the alternative of inviting a third person, and I'm hoping you know someone.
Actually I appreciate the devotion to thorough testing.
================
Comment at: test/FileCheck/check-dag-overlap.txt:467
+; DagNotDag: {{^}}__DagNotDag
\ No newline at end of file
----------------
jdenny wrote:
> jdenny wrote:
> > probinson wrote:
> > > jdenny wrote:
> > > > probinson wrote:
> > > > > Reading this in the overlapping mode: The X set of DAGs will all match on line 424, the Y set will all match on line 425, therefore the NOTs for rule 1 will correctly not match anything. The Z set of DAGs will all match on line 428, therefore the NOTs for rule 2 will correctly not match anything. Then the NOTs for rule 3 will match, making the run fail.
> > > > >
> > > > > Is that what you intended? Naively I would have thought you'd try to break each of the rules, but that's not what the test does.
> > > > > Reading this in the overlapping mode: The X set of DAGs will all match on line 424,
> > > >
> > > > Yes.
> > > >
> > > > > the Y set will all match on line 425
> > > >
> > > > No. y also matches on line 424 because overlapping is permitted, so we get a reordering complaint. You can confirm this with the -vv option.
> > > >
> > > > > Is that what you intended? Naively I would have thought you'd try to break each of the rules, but that's not what the test does.
> > > >
> > > > It breaks the first rule because of -allow-deprecated-dag-overlap.
> > > >
> > > > With non-overlapping matches, it breaks the second rule if I remove the following line from CheckString::CheckDag:
> > > >
> > > > Matches.push_back(Match{MatchPos, MatchPos + MatchLen})
> > > >
> > > > (I had that bug at one point.)
> > > >
> > > > While rule 3 is exercised, I don't know how to break it without breaking rule 1 or 2 because it's a consequence of rules 1 and 2.
> > > >> the Y set will all match on line 425
> > > >
> > > > No. y also matches on line 424 because overlapping is permitted, so we get a reordering complaint. You can confirm this with the -vv option.
> > >
> > > Huh. I would have expected the -NOT directives for rule 1 to break up the DAG lines into separate X and Y groups, and the Y group's starting position would necessarily have been the end of the X group's last match.
> > > At least, that appears to be the intended behavior of the `IdentPatNot` part of the test?
> > > I admit I have not downloaded and applied the patches, I'll try to do that tomorrow.
> > The reordering check is a bit odd, regardless of my patch. It looks for y from the start of X, but it looks for the rest of Y from the end of X. My patch makes sure y doesn't overlap with members of X.
> We never really resolved this issue. Given our agreement about how DAG-NOT-DAG will work in the future (as described in your formal model, and as I've implemented in a patch I still need to post), I believe the only aspects of this test that must change are the comments about reordering detection... because there will be no more reordering detection.
>
> Otherwise, I'm inclined to think this test still makes sense because it exercises overlapping cases that used to be mishandled. Does that make sense to you?
Okay, sounds good.
================
Comment at: utils/FileCheck/FileCheck.cpp:1245
// All subsequent CHECK-DAGs should be matched from the farthest
- // position of all precedent CHECK-DAGs (including this one.)
+ // position of all precedent CHECK-DAGs (not including this one).
StartPos = LastPos;
----------------
jdenny wrote:
> jdenny wrote:
> > probinson wrote:
> > > jdenny wrote:
> > > > probinson wrote:
> > > > > jdenny wrote:
> > > > > > probinson wrote:
> > > > > > > jdenny wrote:
> > > > > > > > Speaking of the reordering check, we should discuss this comment change.
> > > > > > > >
> > > > > > > > I made this change to reflect the implementation, but now I'm wondering if the comment described the desired behavior and the implementation was wrong. I'm not really sure. Moreover, this comment change does not reflect any change made by this patch, so perhaps it should be in a separate patch.
> > > > > > > >
> > > > > > > > The implementation works as follows (with or without this patch as there's no overlapping here). Using the X-Y model from earlier, the implementation seeks non-initial members of Y from the end of X rather than the start of Y. For example, the following succeeds even though the second member of Y matches before the first member of Y:
> > > > > > > >
> > > > > > > >
> > > > > > > > ```
> > > > > > > > X:
> > > > > > > > abc
> > > > > > > >
> > > > > > > > Y:
> > > > > > > > ghi
> > > > > > > > def
> > > > > > > > foo
> > > > > > > > jkl
> > > > > > > >
> > > > > > > > X:
> > > > > > > > CHECK-DAG: {{^}}abc
> > > > > > > >
> > > > > > > > CHECK-NOT: {{^}}foo
> > > > > > > >
> > > > > > > > Y:
> > > > > > > > CHECK-DAG: {{^}}def
> > > > > > > > CHECK-DAG: {{^}}ghi
> > > > > > > > CHECK-DAG: {{^}}jkl
> > > > > > > > ```
> > > > > > > >
> > > > > > > > To make things more confusing, if I move the occurrence of foo right before the occurrence of def, the CHECK-NOT fails. That is, the CHECK-NOTs are sought before the first member of Y, but the other members of Y are sought both before and after Y. It might be more intuitive if the first member of Y marks the beginning of Y (as the comment originally claimed) so that CHECK-NOTs are sought strictly between all of X and all of Y.
> > > > > > > My understanding of putting NOT between DAGs is this:
> > > > > > > ```
> > > > > > > X:
> > > > > > > abc
> > > > > > > Y:
> > > > > > > ghi
> > > > > > > def
> > > > > > > foo
> > > > > > > jkl
> > > > > > >
> > > > > > > X:
> > > > > > > CHECK-DAG: b
> > > > > > > CHECK-DAG: a
> > > > > > >
> > > > > > > CHECK-NOT: foo
> > > > > > >
> > > > > > > Y:
> > > > > > > CHECK-DAG: def
> > > > > > > CHECK-DAG: ghi
> > > > > > > ```
> > > > > > > First the X group of DAGs runs, over the entire input text. The farthest-forward match (here 'b') is remembered.
> > > > > > > Then the Y group of DAGs runs, from the farthest-forward X match (i.e., after 'b') to the end of the input text. The earliest match (here 'ghi') is remembered.
> > > > > > > Finally the NOT runs, in the range from the end of the X matches (after 'b') to the beginning of the earliest Y match (before 'ghi').
> > > > > > >
> > > > > > > The X matches and the Y matches can never overlap, even though they are both DAG groups they are separated by the NOT. Therefore the search range of the Y group is constrained to start after the latest match of the X group.
> > > > > > >
> > > > > > > Is that not what happens? Even with overlapping DAG matches enabled? Your explanation made it sound as if you expected the order of DAG directives in Y to make a difference to the NOT directive, when I think it should not.
> > > > > > > First the X group of DAGs runs, over the entire input text. The farthest-forward match (here 'b') is remembered.
> > > > > >
> > > > > > Agreed.
> > > > > >
> > > > > > > Then the Y group of DAGs runs, from the farthest-forward X match (i.e., after 'b') to the end of the input text.
> > > > > > > The earliest match (here 'ghi') is remembered.
> > > > > >
> > > > > > Only the first DAG in Y runs at this point, so 'def' is remembered.
> > > > > >
> > > > > > > Finally the NOT runs, in the range from the end of the X matches (after 'b') to the beginning of the earliest Y match (before 'ghi').
> > > > > >
> > > > > > Before 'def'.
> > > > > >
> > > > > > > The X matches and the Y matches can never overlap, even though they are both DAG groups they are separated by the NOT. Therefore the search range of the Y group is constrained to start after the latest match of the X group.
> > > > > >
> > > > > > Agreed.
> > > > > >
> > > > > > > Is that not what happens? Even with overlapping DAG matches enabled?
> > > > > >
> > > > > > My patch and -allow-deprecated-dag-overlap shouldn't affect these examples as there is no overlapping.
> > > > > >
> > > > > > > Your explanation made it sound as if you expected the order of DAG directives in Y to make a difference to the NOT directive, when I think it should not.
> > > > > >
> > > > > > It does make a difference, and I agree it's not intuitive.
> > > > > >
> > > > > >
> > > > > It is so unintuitive that I would call it a bug. The question is whether to fix that first, before doing the no-overlapping bit. Do you think it would be straightforward, or a big deal?
> > > > I see two possible fixes:
> > > >
> > > > 1. One approach is what the original comment here suggested: make the first DAG in Y mark the beginning of all Y matches. Basically, for that first directive in Y, CHECK-DAG becomes CHECK. This approach seems very quick to implement. However, it constrains what DAGs in Y can match, so it might break existing tests.
> > > >
> > > > 2. Another approach is to do what you described: match all DAGs in Y and then make the earliest match delimit the end of the preceding NOTs. That will require more work to implement, I think. However, it doesn't constrain what DAGs in Y can match. Instead, it constrains what the preceding NOTs can match, and constraining NOTs helps tests pass not fail. The question is whether they will be false passes that we'll never hear about. My hunch is no.
> > > >
> > > > While #2 seems harder to implement, it seems like it wouldn't causes tests to fail, and I think it produces more intuitive behavior. What do you think?
> > > >
> > > > >> Therefore the search range of the Y group is constrained to start after the latest match of the X group.
> > > > >
> > > > > Agreed.
> > > >
> > > > I got confused here. The search range of the first DAG in Y starts where X's search range starts not where X's matches end. Otherwise, there would be no possibility of a reordering complaint.
> > > > While #2 seems harder to implement, it seems like it wouldn't causes tests to fail, and I think it produces more intuitive behavior. What do you think?
> > >
> > > I think a group of DAGs should not behave differently just because it is preceded by a NOT, and so I still prefer #2.
> > >
> > > > I got confused here. The search range of the first DAG in Y starts where X's search range starts not where X's matches end. Otherwise, there would be no possibility of a reordering complaint.
> > >
> > > That suggests that the first DAG in Y is being mis-handled as part of the X group. I don't think that makes any sense at all. Either a NOT cleanly separates the surrounding DAGs into two groups, or they are all part of one bigger group. Taking the lexically first DAG from the second group and moving it to the first group is just wrong.
> > >
> > > I think a group of DAGs should not behave differently just because it is preceded by a NOT, and so I still prefer #2.
> >
> > Agreed. I'll look into it. If it doesn't break anything, we can put it in before this patch.
> >
> > So that I can keep this straight: The above is about the search range end for the NOTs before Y. The below is about the search range start for Y and thus reordering detection.
> >
> > >> I got confused here. The search range of the first DAG in Y starts where X's search range starts not where X's matches end. Otherwise, there would be no possibility of a reordering complaint.
> > >
> > > That suggests that the first DAG in Y is being mis-handled as part of the X group. I don't think that makes any sense at all. Either a NOT cleanly separates the surrounding DAGs into two groups, or they are all part of one bigger group. Taking the lexically first DAG from the second group and moving it to the first group is just wrong.
> >
> > I agree the current reordering detection is broken. I think the two options you are proposing are the following:
> >
> > 1. Search for every DAG in Y from the end of X's matches until EOF. That would make reordering detection impossible, so we could just get rid of it.
> >
> > 2. Search for every DAG in Y from the start of X's matches until EOF. That way, reordering is detected for any Y member not just the first.
> >
> > Is that what you mean? Which do you prefer?
> I think the only issues left to address in this thread are: (1) remove reordering detection and fix search ranges, and (2) update accordingly the comment that this thread was originally discussing. Again, #1 happens in a later patch I still need to post, and #2 will be addressed there too. Before that patch, the comment change as it appears above is accurate, so I think I should just leave it be for now. For those reasons, I'm marking these comments as done, but let me know if you disagree.
No, that's fair.
https://reviews.llvm.org/D47106
More information about the llvm-commits
mailing list