On Thu, Dec 07, 2017 at 11:03:10AM -0700, Martin Sebor wrote:
> On 12/07/2017 09:55 AM, Jakub Jelinek wrote:
> > On Wed, Dec 06, 2017 at 05:30:53PM +0100, Jakub Jelinek wrote:
> > > On Wed, Dec 06, 2017 at 09:20:15AM -0700, Martin Sebor wrote:
> > > > Attached is a patch with the comment updated/simplified.
> > > > The tests do the job they need to do today so I just removed
> > > > the useless attribute but otherwise left them unchanged. If
> > > > you would like to enhance them in some way please feel free.
> > >
> > > Ok for trunk, with a minor nit. I'll tweak the tests incrementally
> > > when it is in.
> >
> > So here is the fix for those testcases.
> >
> > They didn't test what they meant to test, because they didn't FAIL
> > without the patch. That is because the bug was that the -W* option
> > affected code generation, so with -O2 -Wno-stringop-overflow it didn't
> > trigger it.
>
> Doh! I suppose that underscores that the right way to write test
> cases for optimization bugs is to prune warnings out of their output
No, warnings shouldn't be pruned unless really necessary.
It really depends on what you are testing and whether you want to include
-W* options, or -Wno-*, or none of them, or -w. E.g. the last one is
default in gcc.c-torture/{compile,execute}.
If you need some warnings enabled, then better just add dg-warning
for the options, then you verify you get the right warnings and if it is
dg-do run also right execution, etc.
The reason why you can't have -Wno-foo here is that the test doesn't FAIL
without it without the fix. That should be a standard procedure for every
submitted patch which adds a test testing a fix in the same patch.
With a vanilla compiler test that
make check<whatever> RUNTESTFLAGS=<whatever.exp>=the_test
FAILs and with the patch applied PASSes. This takes just a couple of
seconds. Only after this one should bootstrap/regtest it fully.
> I don't have an opinion on the rest of these changes. I do want
> to make one comment about runtime tests. I fairly regularly run
> tests with cross-compilers on the build machine. This lets me
> verify that compile-only tests pass but it doesn't do anything
> for tests that need to run. In fact, with the current mixture
> of all kinds of tests in the same directory, it pretty much rules
> out drawing any conclusions from test results in this setup. So
> while I appreciate the additional testing done by the runtime
> tests, I think ideally, having compile time only tests would be
> the baseline requirement and runtime tests would be a separate
> layer that would provide additional validation when possible.
Depends on what is being tested, properly written valid (without UB) runtime
tests are extremely useful, they test more than compile time only tests can
check and even in the future often they will catch bugs in other parts of
the compiler. One of the reason why I don't like unit testing too much...
And no, I don't think a compile time test should be added if we are already
adding a runtime test that covers it too and better.
Jakub
