[PATCH] D54769: [FileCheck] New option -warn

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

In D54769#1309251 <https://reviews.llvm.org/D54769#1309251>, @SjoerdMeijer wrote:

> But I still disagree with this one:
>
> > And if the input was this:
> > 
> >   RUN: llc -mtriple=arm-linux-gnueabi %s -o - | FileCheck %s -check-prefix=FOO
> >   define void @FOO() {
> >   entry:
> >   ; FOO-LABEL: FOO:
> >   ; DOO:             mov  pc, lr
> >   ; FOO:             ret void
> >   }
> > 
> > there is no reasonable way to flag the DOO line as a mistake. In fact, typos in the prefix are a tactic I've often used to "comment out" directives, for one reason or another, and I would not want that tactic to start triggering warnings.
>
> I think this is the prime example where I would like to see and get a diagnostic. In this example, I think there are 2 bugs: 1 in the test, and 1 in FileCheck. There are probably many ways to temporarily comment out things and this is a "dangerous" one. Because allowing this relaxed FileCheck behaviour  it's too easy to shout ourselves in the foot, and that's why we have so many test issues. The fact that we have so many test issues, shows that code review isn't always catching this. But I don't think code review should not really have to deal with this. We have a tool for this: FileCheck should do its checking, and do it properly. Besides 'commenting out' checks in this way, the other cause for this if you first had `--check-prefixes=FOO,DOO`, then some test refactoring happens, `DOO` is dropped from the `--check-prefixes` line, and the test becomes half useless, half correct.


And if the input was this:

  ; RUN: llc -O2 %s -o - | FileCheck %s -check-prefix=FOO
  ; RUN: llc -O0 %s -o - | FileCheck %s -check-prefixex=FOO,DOO
  define void @FOO() {
  entry:
  ; FOO-LABEL: FOO:
  ; DOO:             noopt-only-case
  ; FOO:             opt-or-noopt-case
  }

Then we absolutely don't want a warning for DOO on the first run.  This test is written correctly.


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

https://reviews.llvm.org/D54769