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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Ok, nice, looks like we actually agree: we want this to be errors, not warnings. Perhaps I conflated things too, and this was likely caused for practical reasons: how do we get all tests error free so that we the we can enable the error checking?

Just to reiterate that 1) there are serious test problems, and 2) this is quite a bit of work, below the list of things I've cleaned up and quite importantly, I have only looked in `test/CodeGen/ARM/`:

- https://reviews.llvm.org/rL345386
- https://reviews.llvm.org/rL345491
- https://reviews.llvm.org/rL347487
- and I haven't fixed yet: `ARM/2011-11-07-PromoteVectorLoadStore.ll`, `ARM/2011-11-09-BitcastVectorDouble.ll`, `ARM/fast-isel-update-valuemap-for-extract.ll`, and `ARM/thumb2-size-reduction-internal-flags.ll`.

I just had a quick look and in the entire test suite (with backends X86, AArch64 and ARM enabled) there are ~200 tests where this unused-prefix triggers.

Therefore, I hope Joel's proposed approach is acceptable; I agree with that and I see that as a convenient way to gradually clean up and fix tests.

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.


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

https://reviews.llvm.org/D54769