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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In https://reviews.llvm.org/D54336#1294282, @chandlerc wrote:

> 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!



================
Comment at: lib/Support/FileCheck.cpp:586-588
+  if (NextChar == ':') {
+    return {Check::CheckPlain, Rest};
+  }
----------------
chandlerc wrote:
> 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)
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.


================
Comment at: lib/Support/FileCheck.cpp:707
   // We ran out of buffer while skipping partial matches so give up.
-  return StringRef();
+  return {StringRef(), StringRef()};
 }
----------------
chandlerc wrote:
> 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.
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).


Repository:
  rL LLVM

https://reviews.llvm.org/D54336