[PATCH] D54336: [FileCheck] introduce CHECK-COUNT-<num> repetition directive
Chandler Carruth via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Nov 10 09:34:47 PST 2018
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
More information about the llvm-commits
mailing list