Page MenuHomePhabricator

[FileCheck] Add directive for checking for blank lines
ClosedPublic

Authored by jhenderson on Jan 19 2017, 3:20 AM.

Details

Summary

There is currently no reliable way in FileCheck to check that a line is completely empty. The expected way of using "CHECK: {{^$}}" does not work because the '^' matches the end of the previous match (this behaviour may be desirable in certain instances). For the same reason, "CHECK-NEXT: {{^$}}" will fail when the previous match was at the end of the line, as the pattern will match there. Using the recommended [[:space:]] to match an explicit new line could also match a space, and thus is not always desired. Literal '\n' matches also do not seem to work.

This change adds a new directive that behaves the same as CHECK-NEXT, except that it only matches against empty lines. As with CHECK-NEXT, it will fail if more than one newline occurs before the next blank line. Example usage:
; test.txt
foo

bar
; CHECK: foo
; CHECK-EMPTY:
; CHECK-NEXT: bar

Diff Detail

Repository
rL LLVM

Event Timeline

jhenderson created this revision.Jan 19 2017, 3:20 AM
davidb added a subscriber: davidb.Jan 19 2017, 4:10 AM
probinson edited edge metadata.Jan 19 2017, 4:20 PM

I spent some time experimenting with different approaches, and my inner hacker is pleased to find something unobvious that works. Admittedly it is not as cleanly self-documenting as the new directive.

CHECK:foo{{$}}
CHECK-SAME:{{[[:space:]]$}}
CHECK-NEXT:bar

which must be run using the --strict-whitespace option to FileCheck, so that the :space: cannot match anything other than the newline between the two $ metacharacters. Also note the second directive is SAME not NEXT. Without --strict-whitespace then additional spaces on the "empty" line would be allowed. That option is really for extra-picky cases, but if you are caring about blank lines then it's pretty clear you already have an extra-picky case. :-) Let me know if it doesn't work for you, or if you have other cases where this tactic would not be usable.

Alternatively, we could figure out how to get the regex package to recognize \n (and for that matter other backslash-escape sequences). This has come up before, so I think that would be more valuable in the long run than adding another FileCheck directive.

I just ran into this problem writing a test case and for now I used the workaround by @probinson but having a CHECK-EMPTY directive would make it much cleaner.

jeroen.dobbelaere added a comment.EditedJun 21 2018, 3:09 AM

How about using an empty 'CHECK-NEXT:' statement for this ?

; CHECK:foo
; CHECK-NEXT:  
; CHECK-NEXT:bar

That would make it much easier to copy-paste a reference piece, add 'CHECK-NEXT:' in front, except for the first line, and be done with it.

How about using an empty 'CHECK-NEXT:' statement for this ?

; CHECK:foo
; CHECK-NEXT:  
; CHECK-NEXT:bar

That would make it much easier to copy-paste a reference piece, add 'CHECK-NEXT:' in front, except for the first line, and be done with it.

What would be the meaning of CHECK-NEXT in this situation? Because that would to me, match an empty string, expected to be on the next line. The next empty string after foo is trivially on the same line as foo. Note that CHECK-NEXT searches for the next instance of the specified string, and fails if it finds it anywhere other than on the next line, rather than just searching the next line for the specified string.

This change would be really really useful for addressing http://llvm.org/PR37871 so I'd like to have this.

The llvm-mca output typically consists of logical "blocks" of info delimited by empty lines:

Foo
Bar
Baz

Wibble
Wobble

so our tests model this like:

CHECK:      Foo
CHECK-NEXT: Bar
CHECK-NEXT: Baz

CHECK:      Wibble
CHECK-NEXT: Wobble

which is fine for making sure that the contents of each of these blocks is correct but will miss the case where there is something unexpected in between those blocks. Being able to instead say:

CHECK:      Foo
CHECK-NEXT: Bar
CHECK-NEXT: Baz

CHECK-EMPTY:

CHECK-NEXT: Wibble
CHECK-NEXT: Wobble

would be very useful!

Bowing to popular demand.

test/FileCheck/check-empty-tag.txt
13 ↗(On Diff #84952)

I believe this CHECK2A: line will match itself, and the "next line" therefore has a CHECK2A-EMPTY directive on it. If you run FileCheck manually I think you'll see that from the line number reported by the error.
I see that you are intending to match a line at the end of this file. To achieve that you want to add some kind of regex to the CHECK2A line. Something like the {{next}} line has spaces would work.

utils/FileCheck/FileCheck.cpp
470 ↗(On Diff #84952)

Spurious whitespace difference.

755 ↗(On Diff #84952)

Disallow EMPTY-NOT and NOT-EMPTY.

867 ↗(On Diff #84952)

Spurious reformatting?

jhenderson marked 3 inline comments as done.
jhenderson set the repository for this revision to rL LLVM.
jhenderson added a subscriber: edd.

Address review comments:

  • Rebased (no actual changes needed as a result, but it updates the diff context).
  • Removed unrelated whitespace changes.
  • Move CHECK2 to be the last case in the test, so that it is closer to the strings to be checked.
  • Added checking for CHECK-EMPTY-NOT and CHECK-NOT-EMPTY patterns.
test/FileCheck/check-empty-tag.txt
13 ↗(On Diff #84952)

I believe this CHECK2A: line will match itself, and the "next line" therefore has a CHECK2A-EMPTY directive on it. If you run FileCheck manually I think you'll see that from the line number reported by the error.

I thought of that at the time I originally wrote this, but it doesn't because of the "--match-full-lines" switch, since the "CHECK2A:" prefix prevents matching there.

I'm not sure I follow the need for a regex?

utils/FileCheck/FileCheck.cpp
867 ↗(On Diff #84952)

This came about because that's what my clang-format did, but you're right, it's not related to this change.

probinson accepted this revision.Jun 26 2018, 7:39 AM

One doc question, otherwise LGTM.

docs/CommandGuide/FileCheck.rst
247 ↗(On Diff #152881)

Hm. Is whitespace allowed on the "blank" line? Even if --match-full-lines is not in effect? If whitespace is always disallowed, then maybe instead of "is blank" this should say something like "has nothing on it, not even whitespace" ?

test/FileCheck/check-empty-tag.txt
13 ↗(On Diff #84952)

A regex is a way for a CHECK line to avoid matching itself; you see this in some of the other FileCheck tests. But with --match-full-lines you don't need that trick; sorry I missed that.

This revision is now accepted and ready to land.Jun 26 2018, 7:39 AM
jhenderson added inline comments.Jun 26 2018, 7:48 AM
docs/CommandGuide/FileCheck.rst
247 ↗(On Diff #152881)

At the moment, whitespace isn't allowed (see case 6 in the test). This could probably be changed, if there's call for it, but I figured that it was simpler this way.

I'll change the wording as suggested.

This revision was automatically updated to reflect the committed changes.