Page MenuHomePhabricator

[FileCheck] introduce CHECK-COUNT-<num> repetition directive
ClosedPublic

Authored by fedor.sergeev on Nov 9 2018, 11:17 AM.

Details

Summary

In some cases it is desirable to match the same pattern repeatedly
many times. Currently the only way to do it is to copy the same
check pattern as many times as needed. And that gets pretty unwieldy when
count is high enough that counting these patterns at a single glance is nontrivial.

Introducing CHECK-COUNT-<num> directive which acts like a plain CHECK
directive yet matches the same pattern exactly <num> times.

Extended FileCheckType to a struct to add Count there.
Changed some parsing routines to handle non-fixed length of directive
(all currently existing directives were fixed-length).

The code is generic enough to allow future support for COUNT in more
than just PlainCheck directives.

See motivating example for this feature in reviews.llvm.org/D54223.

Diff Detail

Repository
rL LLVM

Event Timeline

fedor.sergeev created this revision.Nov 9 2018, 11:17 AM

I know, it needs documentation changes :)

Seems pretty reasonable to me. Pretty minor things below...

include/llvm/Support/FileCheck.h
62 ↗(On Diff #173392)

nit: blank line here

69 ↗(On Diff #173392)

I'd prefer to use an int throughout for this.

lib/Support/FileCheck.cpp
587 ↗(On Diff #173392)

int64_t?

855 ↗(On Diff #173392)

clang-format

addressing comments, clang-formatted, added std includes, some minor stuff

fedor.sergeev marked 4 inline comments as done.Nov 9 2018, 12:36 PM
fedor.sergeev edited the summary of this revision. (Show Details)Nov 9 2018, 12:40 PM

addings docs

dblaikie added inline comments.Nov 9 2018, 5:47 PM
lib/Support/FileCheck.cpp
586–588 ↗(On Diff #173431)

Bit inconsistent (with LLVM style, and within this file specifically) to have {} on this single-line block (& another further down (consumeInteger))?

707 ↗(On Diff #173431)

Does this work as {"", ""}? or does that do the wrong thing/not compile?

763–766 ↗(On Diff #173431)

I'd probably use a conditional operator here, but not a huge deal by any means if you prefer it this way.

fedor.sergeev added inline comments.Nov 10 2018, 12:49 AM
lib/Support/FileCheck.cpp
586–588 ↗(On Diff #173431)

This one - you'r right.
consumeInteger - I wonder, I do have one comment and one statement there, does it count as a single-line block?

707 ↗(On Diff #173431)

Direct shorter equivalent would be
{ {}, {} }
And this one:
{ "", "" }
is a tad different, by providing a non-null data pointer (to empty constant string).
For the purpose of this change all these are the same.

As you see, originally there was just StringRef(), thats why I chose my version.
I dont feel strongly about either of these versions.

minor cleanup

chandlerc accepted this revision.Nov 10 2018, 9:34 AM

FWIW, this looks really good to me. I'd be fine with this going in as-is and any further improvements suggested by David as follow-ups. Feel free to submit with comments addressed.

lib/Support/FileCheck.cpp
586–588 ↗(On Diff #173431)

I personally usually omit braces unless it is in a surrounding context that makes this deeply confusing. (weird loop nesting, dense other conditions, etc)

707 ↗(On Diff #173431)

FWIW, I prefer something other than {"", ""}. I have a mild preference for {StringRef(), StringRef()}, but it is very mild.

This revision is now accepted and ready to land.Nov 10 2018, 9:34 AM
fedor.sergeev marked 4 inline comments as done.Nov 11 2018, 1:59 AM

FWIW, this looks really good to me. I'd be fine with this going in as-is and any further improvements suggested by David as follow-ups. Feel free to submit with comments addressed.

Yeah, nothing more from me - thanks!

lib/Support/FileCheck.cpp
586–588 ↗(On Diff #173431)

Yeah, opinions differ here - some folks only omit braces on single line scopes, some omit them from any single statement (whether or not the statement is wrapped over multiple lines, or there are comment lines, etc). I don't feel strongly either - I just hadn't noticed/thought about the fact that the other was multi-line-single-statement block.

707 ↗(On Diff #173431)

Fair enough, I don't feel too strongly (if there's any specific motivation for the aversion to {"", ""} I'd be curious to hear it, for reference/understanding/etc).

fedor.sergeev marked 4 inline comments as done.Nov 12 2018, 2:57 PM
fedor.sergeev added inline comments.
lib/Support/FileCheck.cpp
707 ↗(On Diff #173431)

One particular motivating aspect is that "" creates a special object and that is only to create an empty StringRef which does not need anything than just length == 0.

Asked people on IRC and none of them chose "","" variant ;)
Also, Zygoloid mentioned that { "", "" } could be treated as a call to constructor that takes two iterators (there is none such constructor presently though).

Anyway I will go for the most uncontroversial StringRef() version, despite it being somewhat chatty. I dont mind typing a few extra characters :)

fedor.sergeev marked 3 inline comments as done.Nov 12 2018, 2:58 PM
This revision was automatically updated to reflect the committed changes.