On Thu, 2015-11-19 at 18:35 +0100, Bernd Schmidt wrote:
> In general I'm much happier with this approach, and I think this series
> is close to ready, but I want to bring up some questions that could use
> wider discussion.
> > This patch adds a selftest.h/.c to gcc, with an API loosely
> > modelled on gtest (though without the use of CamelCase): it
> > supports enough of the gtest API to enable the tests that I
> > wrote to run with minimal changes.
>
> Here there's a question of style. I don't want to approve or reject this
> just now, I'd like to hear what others think. To my eyes this still
> looks rather seriously overengineered. Plain gcc_assert and if (cond)
> abort (); would work just fine for the tests IMO, it's what we have for
> regular testcases where we don't distinguish between all sorts of
> microscopic subtests, and any gcc_assert failure is easy enough to
> debug, just load up cc1 into the debugger and run. I don't think we need
> output for tests that are really just expected to pass always, all we
> need is the build to stop if an internal error is detected.
gcc_assert terminates the process and no further testing is done,
whereas the approach the kit tries to run as much of the testsuite as
possible, and then fail if any errors occurred.
Given that the aim is for something that runs very quickly in stage1 and
should rarely fail, this distinction may be irrelevant, though maybe the
latter approach is better for the case where *something* has broken a
lot of tests and you want to see what the lowest level failures are,
rather than just the first one that broke.
Consider the case of something being tested on, say x86_64, that works
fine there, but which breaks some of the selftests on AIX (perhaps the
selftest failure is indicating a genuine problem, or perhaps the
selftest is over-specified). I believe it would be preferable to have
some sort of report on the scope of the breakage, rather than require
each test to be fixed before the self-test can continue.
> If I'd written it I'd also have used a somewhat lower-tech approach for
> the registration and running of tests, but once again I'd like to hear
> from others.
The patch kit does use a lot of "magic" via macros and C++.
Taking registration/discovery/running in completely the other direction,
another approach could be a completely manual approach, with something
like this in toplev.c:
bitmap_selftest ();
et_forest_selftest ();
/* etc */
vec_selftest ();
This has the advantage of being explicit, and the disadvantage of
requiring a bit more typing. I was going to add that there might be
more risk of adding a test and not having it called, but I suspect that
the "function declared but not used" warning would protect us from that.
(there's also the loss of the ability to e.g. randomize the order in
which the tests get run, but that's less important).
(possibly passing in a "selftest *" param if we're doing the
try-to-run-everything approach, so we can count failures etc without
introducing globals)
Was that what you had in mind, or something else?
> For things like
>
> > +#define RUN_ALL_TESTS() \
> > + ::selftest::run_all_tests ()
>
> I don't see the point of the macro.
FWIW, this was for compatibility with the gtest API, but yeah, it's
redundant.
>Also, in [8/15]
>
> > +class gimple_test : public ::selftest::test
> > +{
> > + protected:
> > + void
> > + verify_gimple_pp (const char *expected, gimple *stmt)
> > + {
> > + pretty_printer pp;
> > + pp_gimple_stmt_1 (&pp, stmt, 0 /* spc */, 0 /* flags */);
> > + EXPECT_STREQ (expected, pp_formatted_text (&pp));
> > + }
> > +};
> > +
>
> Why have the class rather than just a function? This sort of thing makes
> me go "overuse of C++".
It's useful if a test has state: the test functions are methods of a
class, so the state can live as instance data, and hence the helper
functions in the fixtures can get at it. But this could be achieved in
other ways that might be more explicit, and use less macro chicanery.
(also, the result reporting relies on the test code being a method of
a ::selftest::test subclass so it's useful to write fixtures as
subclasses to inherit from, but yeah, this could be overengineering it).
Thanks
Dave
