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

Mon Nov 26 15:59:51 PST 2018

jdenny added a comment.

In D54769#1308788 <https://reviews.llvm.org/D54769#1308788>, @dblaikie wrote:

> In D54769#1308775 <https://reviews.llvm.org/D54769#1308775>, @probinson wrote:
>
> > In D54769#1308690 <https://reviews.llvm.org/D54769#1308690>, @jdenny wrote:
> >
> > > In D54769#1308654 <https://reviews.llvm.org/D54769#1308654>, @dblaikie wrote:
> > >
> > > > 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?
> > >
> > >
> > > Adding -warn now makes it easier for anyone to contribute to this cleanup and doesn't hurt anyone who doesn't care about it.
> >
> >
> > I wonder if `-allow-deprecated-dag-overlap` is a better model: an error that can be explicitly disabled.  The idea there was that over time we could rework the buggy tests, and when that's done, remove the option.  In the meantime all new tests will get complaints, which is what we want.
>

In that case, before we proceed, we need a much more comprehensive survey of the effect of these diagnostics on all existing test suites, as I did for -allow-deprecated-dag-overlap.

Making this diagnostic a warning for now gives us a gentler way to ease into this: we add the warning, we explore the effect, and then we decide whether to (1) remove the warning, (2) leave it as a warning for some test suites and for manual usage, or (3) make it an error.

Moreover, -allow-deprecated-dag-overlap was a case where we were fixing subtle behavior and not simply enabling/disabling a diagnostic.  We had strong evidence that the old behavior was confusing users, and supporting two different behaviors forever would surely be considerably more confusing.  In contrast, it's not clear at all to me that the diagnostics we're discussing now are universally correct and thus should become errors.  We already discussed that point at length for the diagnostic introduced by D53710 <https://reviews.llvm.org/D53710>.

For the diagnostic introduced by this patch (individually unused prefixes), I've written tests (locally, not sure about upstream) where I purposely didn't always use all prefixes specified to FileCheck.  Specifically, I have tests in which I'm trying to exercise many different combinations of some executable's features: foo, bar, etc.  Such a test has many FileCheck calls, each  checking output when some particular combination of features is enabled, and I have a systematic FileCheck prefix scheme that corresponds to those combinations: FOO-MAYBE-BAR, BAR-MAYBE-FOO, FOO-NOT-BAR, FOO-AND-BAR, etc.  As the test code evolves, the prefixes specified in those FileCheck calls rarely need to change, but which prefixes are used in actual directives do change.  If I had to go back and update the FileCheck calls every time the usage changes, I'd probably forget to add back prefixes, and my test would pass when it shouldn't.  It's better to write the FileCheck calls with their full sequences of prefixes once and not have to update them again.

Now, I can't argue we need to cater to that use case until it's upstream, and someone might offer a better way to accomplish what I want to accomplish in those tests.  But my point is that there logically exists a use case where people could be depending on this diagnostic not being an error, and maybe it even makes sense that, for those cases, this diagnostic should never become an error.  We'll only know if we explore more test suites.  A warning (especially if we also provide something like -Werror) is the easiest way to do that, in my opinion.

>> I think @dblaikie has a point; if we want to move to a new world with less-fallible tests, it's better for new FileCheck diags to be errors that are on-by-default, and explicitly disabled in the tests that we haven't managed to fix yet.

I'd agree if we already knew that all tests producing the diagnostics likely deserve to be fixed.  But we haven't looked hard enough yet, in my opinion.

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

https://reviews.llvm.org/D54769