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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D54769#1308542 <https://reviews.llvm.org/D54769#1308542>, @jdenny wrote:

> In D54769#1308317 <https://reviews.llvm.org/D54769#1308317>, @dblaikie wrote:
>
> > In D54769#1306275 <https://reviews.llvm.org/D54769#1306275>, @SjoerdMeijer wrote:
> >
> > > I decided to continue here, and implemented the diagnostic that Paul suggested in the other ticket, because it's small and a nice demonstrator for this new option:
> > >
> > > > Another useful diagnostic (probably best done separately) would be when a given prefix triggers no directives at all. That's obviously an incorrect test; either it has typos or has a dangling -check-prefix whose directives have all been removed. Right now, this diagnostic occurs only when there are no matching directives at all. But if you specify two prefixes, and only one of them matches a directive, FileCheck is silent. I think that's bad.
> >
> >
> > Sounds like that could be an error, rather than a warning, perhaps?
>
>
> We can always do that later if we decide the warning is reasonable enough of the time.


Mostly I'm pushing back on the general idea of adding a warning feature because of the level of noise that may develop if it's not held as a hard requirement.

>> (in general I'm a bit hesitant to add warnings - either these checks are things we can all agree on are erroneous & should be a failure, or warning cruft will accumulate and make the noise too high for the warning signals to be seen?)
> 
> The usage we discussed is running with FILECHECK_OPTS="-warn" while developing/debugging tests, but normally you'd leave it off.  Does that address your concern?

Not really - either everyone's using it and the tests are warning clean, or not everyone's using it (especially likely early on if it's opt-in) & the codebase has lots of warnings in it that make the feature hard to use practically by those who want to (because every time they sync and build, they'll get spurious warnings they have to clean up to be able to see their own warnings)

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

> >> Sounds like that could be an error, rather than a warning, perhaps?
>
> While I agree with the observation, practically it would a lot easier to start small with a warning, clean things up, and then either promote warnings into errors, or make this particular warning an error. I've spent quite some time just looking at only 2 subdirectories, and fixed only one. Making this an error now would break tests, so we would need one big-bang commit to fix all tests first. I am okay spending time on this, but this is going to take some time, and also for some broken codegen tests I would probably need advise.


Are the warnings you have in mind regressing at a sufficiently high rate to need this, though? (& if other people aren't opting in to warnings-as-errors, how will they be cleaned up?) You/others could checkin fixes as they find them rather than accumulating one large patch - then when it's close to done, you send out the patch to add the error to avoid regressions?


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

https://reviews.llvm.org/D54769