Page MenuHomePhabricator

[docs] Document pattern of using CHECK-SAME to skip irrelevant lines
ClosedPublic

Authored by rupprecht on Sep 25 2019, 4:05 PM.

Details

Summary

This came up during the review for D67656. It's nice but also subtle, so documenting it as an idiom will make tests easier to understand.

Diff Detail

Event Timeline

rupprecht created this revision.Sep 25 2019, 4:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2019, 4:05 PM
MaskRay added inline comments.Sep 25 2019, 10:23 PM
llvm/docs/CommandGuide/FileCheck.rst
427

Maybe align 1 to the position 2 columns right of Value: above.

thopre added inline comments.Sep 26 2019, 1:15 AM
llvm/docs/CommandGuide/FileCheck.rst
384

Nit: rather than say that it is common, it might be better to say that CHECK-SAME is useful in dealing with X. Whether it is common or not does not seem very relevant.

jhenderson added inline comments.
llvm/docs/CommandGuide/FileCheck.rst
384

Yeah, I agree with this. I wrote it for the first time a few months ago, and as far as I know, it was the first time it was used for this purpose (certainly I came up with it independently!), so "common" seems like a stretch at the moment.

jhenderson added inline comments.Sep 26 2019, 1:51 AM
llvm/docs/CommandGuide/FileCheck.rst
429–430

Maybe this would be better phrased as "This verifies that the next time "`Value:`" appears in the output, it has a value of 1.

It might also be worth a note saying to use {{$}} immediately after too, to avoid a spurious match on Value: 16 or similar. Not sure if it really belongs here, or somewhere else though.

MaskRay added inline comments.Sep 26 2019, 2:02 AM
llvm/docs/CommandGuide/FileCheck.rst
429–430

The suggestion looks good. So the example should be

CHECK:      Value:
CHECK-SAME:        1{{$}}
rupprecht marked 8 inline comments as done.
  • Remove mention of "common" case
  • Make the bit about the next line containing Value: more clear
  • Fix some indentation
llvm/docs/CommandGuide/FileCheck.rst
384

Fair enough, I just figured everyone besides me was familiar with this trick :)

427

Did you mean 1 column (from the colon)?

429–430

I added {{$}} to the original matcher, it should be there too

greened added inline comments.
llvm/docs/CommandGuide/FileCheck.rst
418

This is what CHECK-LABEL is for. This isn't a very motivating example. Is there a better one?

rupprecht added inline comments.Sep 26 2019, 11:56 AM
llvm/docs/CommandGuide/FileCheck.rst
418

I don't think CHECK-LABEL works in this case. CHECK-LABEL requires unique lines, and the only thing unique here would be "Name: <...>" which is not the thing we have a problem checking.

e.g. something like:

CHECK: Name: foo
CHECK-LABEL: Value: 1

would fail on the example text above because foo and baz both have a value of one. Similarly,

CHECK-LABEL: Name: foo
CHECK: Value: 1

has the same false-passing problem as described in these added docs.

Of course, you could add tests for every name/value pair (and using check-label on the names to make error messages clearer), but then the test overall becomes not a unit test: you really just want to write a unit test that verifies foo has a value of 1; you don't want to assert the universe to do that.

I don't use check-label very often, I'm mostly relying on docs for my understanding. Could you clarify your comment if I'm missing something? Also: if I'm not missing something, let me know how I can make the wording here more clear.

thopre added inline comments.Sep 27 2019, 12:59 AM
llvm/docs/CommandGuide/FileCheck.rst
418

CHECK-LABEL will constraints the CHECK directive between them to match between the two posititions where the CHECK-LABEL match. In other word, if the name in your example are uniques then

CHECK-LABEL: Name: foo
CHECK: Value: 1
CHECK-LABEL: Name: bar

will guarantee that the Value: 1 will match something in the block of text between "Name: foo" and "Name: bar" or fail to match. Now if you only care about foo, your solution avoids adding a CHECK-LABEL for bar which might not be easy if you do not know the relative ordering of the blocks for foo and bar.

So I do think your use case is useful, but perhaps you should remove the parts about bar and baz (my preference) or mention that the order of blocks cannot be relied upon (which might be solved in some future time by having something like CHECK-LABEL-DAG/CHECK-REGION-DAG which would force to update this text to my first suggestion).

jhenderson added inline comments.Sep 27 2019, 1:21 AM
llvm/docs/CommandGuide/FileCheck.rst
418

Perhaps replacing the reference to baz with something like "a later symbol in the output" would be sufficient? It might also be worth noting why CHECK-LABEL isn't appropriate in this case (e.g. unknown ordering).

probinson added inline comments.Mar 5 2020, 7:24 AM
llvm/docs/CommandGuide/FileCheck.rst
427

This will also match Value: 21 so it is not equivalent to
CHECK: Value: 1{{$}}

thopre added inline comments.Mar 5 2020, 8:01 AM
llvm/docs/CommandGuide/FileCheck.rst
427

True, could this dealt by using CHECK: Value: {{ 1$}}?

probinson added inline comments.Mar 5 2020, 11:26 AM
llvm/docs/CommandGuide/FileCheck.rst
427

Yes, that would work. Nice to have smart people around. :-)

probinson accepted this revision.Jul 28 2020, 7:38 AM

@rupprecht if you are still motivated to land this after so long, the only thing I see missing is to update the example from CHECK-SAME: 1{{$}} to CHECK-SAME: {{ 1$}} and then it LGTM. If not, there seems to be enough interest that someone else can land it?

llvm/docs/CommandGuide/FileCheck.rst
418

CHECK-LABEL technically does not require unique lines, any more than it requires the pattern to be a label. So the sequence

CHECK: Name: foo
CHECK-LABEL: Value: 1{{$}}

would not fail in the case where there were multiple Value: 1 lines; the LABEL would match the first one.
However, the test would still allow false passes, for example if foo had value 2 and bar had value 1, the CHECK-LABEL would match on the Value for bar and Name: foo would still precede it.

An alternative would be

CHECK-LABEL: Name: foo
CHECK: Value: 1{{$}}
CHECK-LABEL: Name:

which would constrain the CHECK to be between foo and whatever came next.

But, I don't think that's inherently better than

CHECK: Name: foo
CHECK: Value:
CHECK-SAME: {{ 1$}}

Both sequences will work, and whether to prefer LABEL or SAME depends partly on context of the output (is there a good LABEL candidate?) and the test itself (is this a single case or are we looking at a series?).

All this is to say, I think the motivation is okay as is.

This revision is now accepted and ready to land.Jul 28 2020, 7:38 AM
thopre added a comment.Aug 5 2020, 3:06 AM

@rupprecht if you are still motivated to land this after so long, the only thing I see missing is to update the example from CHECK-SAME: 1{{$}} to CHECK-SAME: {{ 1$}} and then it LGTM. If not, there seems to be enough interest that someone else can land it?

Done. Please shout if I made a mistake.