[PATCH] D54336: [FileCheck] introduce CHECK-COUNT-<num> repetition directive

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

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.



================
Comment at: lib/Support/FileCheck.cpp:586-588
+  if (NextChar == ':') {
+    return {Check::CheckPlain, Rest};
+  }
----------------
fedor.sergeev wrote:
> dblaikie wrote:
> > Bit inconsistent (with LLVM style, and within this file specifically) to have {} on this single-line block (& another further down (consumeInteger))?
> 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?
> 
> 
I personally usually omit braces unless it is in a surrounding context that makes this deeply confusing. (weird loop nesting, dense other conditions, etc)


================
Comment at: lib/Support/FileCheck.cpp:707
   // We ran out of buffer while skipping partial matches so give up.
-  return StringRef();
+  return {StringRef(), StringRef()};
 }
----------------
fedor.sergeev wrote:
> dblaikie wrote:
> > Does this work as {"", ""}? or does that do the wrong thing/not compile?
> 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.
FWIW, I prefer something other than `{"", ""}`. I have a mild preference for {StringRef(), StringRef()}, but it is very mild.


Repository:
  rL LLVM

https://reviews.llvm.org/D54336