[PATCH] D77227: [RFC][FileCheck] Require colon immediately after CHECK directives

Wed Apr 1 12:22:41 PDT 2020

jdenny added a comment.

In D77227#1955433 <https://reviews.llvm.org/D77227#1955433>, @jroelofs wrote:

> In D77227#1955284 <https://reviews.llvm.org/D77227#1955284>, @jdenny wrote:
>
> > Thanks for working on this.
> >
> >   CHECK           gotcha 1
> >
> >
> > This case seems like it would trigger too many false positives.  Do you have numbers on how many true positives that this case alone catches and that are not caught by the other cases?
>
>
> I don't yet, but I'd be happy to take some measurements/estimates.
>
> Subjectively, it was pretty bad. There are a lot of CHECK's without colons in comments (rightfully so).

I was trying to gauge whether this case is worth the effort.  @jdoerfert called out several true positives.  If those are representative, then maybe it is worthwhile... at least in some form.

> One solution to that might be to add a FileCheck-specific comment sequence,

I think that idea came up once before.  It might also be useful when you want to temporarily comment out a directive.  It would be better than the current practice of mangling the directive.

> though I don't imagine that being very ergonomic to use, and wouldn't help with all the existing tests that have these false positive comments.

Agreed.

>> 
>> 
>>   CHECK :         gotcha 2
>> 
>> 
>> As long as only whitespace separates `CHECK` and `:`, this one seems ok.
> 
> Ok in what sense? If there is any whitespace between the check directive and the colon, FileCheck does not trigger on it. That, to me, makes it look like a valid test that doesn't actually doing anything. Whether this is intentional or not isn't particularly clear to me.

I'm assuming that the whitespace is likely to be unintentional and that the line is meant to be a directive.  We really need numbers for existing test suites to determine the likelihood it's a false positive instead.  If so, then it needs to be opt-in (and perhaps in a separate script as I mentioned earlier).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77227/new/

https://reviews.llvm.org/D77227