This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Selftest framework (v7)
- From: David Malcolm <dmalcolm at redhat dot com>
- To: Bernd Schmidt <bernds_cb1 at t-online dot de>, gcc-patches at gcc dot gnu dot org
- Cc: Bernd Schmidt <bschmidt at redhat dot com>, Jeff Law <law at redhat dot com>
- Date: Mon, 06 Jun 2016 10:17:08 -0400
- Subject: Re: [PATCH] Selftest framework (v7)
- Authentication-results: sourceware.org; auth=none
- References: <5750BF79 dot 20201 at redhat dot com> <1464981166-25871-1-git-send-email-dmalcolm at redhat dot com> <57540F16 dot 8050504 at t-online dot de>
On Sun, 2016-06-05 at 13:37 +0200, Bernd Schmidt wrote:
> On 06/03/2016 09:12 PM, David Malcolm wrote:
> > It's not clear to me if these approvals still hold.
>
> I was willing to go with it; I had a look through some of these
> patches
> and didn't spot anything untoward. To make it clear, this patch is
> OK,
> with one tweak if possible: extend the namespace selftest to cover
> the
> various helper functions (some of these have names like from_int
> which
> ideally we wouldn't leak into the rest of the compiler).
I believe that apart from the from_int specializations, everything else
is marked with "static" (we can't mark template specializations with
"static").
> As far as I can
> tell this just involves moving the start of namespace selftest
> upwards a
> bit in the files where we have tests.
Yes, and it does seem cleaner to have all of the selftest code start
like this:
#if CHECKING_P
namespace selftest {
I'll make that change, apart from in diagnostic-show-locus.c, where the test functions are already within an anonymous namespace (the one containing the implementation).
> A few other minor things...
>
> > + tree bind_expr =
> > + build3 (BIND_EXPR, void_type_node, NULL, stmt_list, block);
>
> Operators go at the start of the line.
Fixed.
> > + tree fn_type = build_function_type_array (integer_type_node, /*
> > return_type */
>
> The line is too long, and we don't do /* arg name */ anyway.
Fixed.
> > +static void
> > +assert_loceq (const char *exp_filename,
> > + int exp_linenum,
> > + int exp_colnum,
> > + location_t loc)
>
> > +static layout_range
> > +make_range (int start_line, int start_col,
> > + int end_line, int end_col)
>
> These lines are too short :) Could save some vertical space here.
Fixed.
> For the future - I found the single merged patch easier to deal with
> than the 16- or 21-patch series. Split ups are often good when
> modifying
> the same code in multiple logically independent steps (keeping in
> mind
> that bugfixes to newly added code shouldn't be split out either).
> This
> is a different situation where the patches weren't truly independent,
> and the merged patch is essentially just a concatenation, so
> splitting
> it up does not really make the review any easier (potentially harder
> if
> you have to switch between mails rather than just hitting PgUp/Dn.
OK. Sorry about that.
I'm testing a revised patch now, incorporating the above, and renaming
s-selftests (plural) to s-selftest (singular) etc within
gcc/Makefile.in as requested by Bernhard elsewhere in this thread. I
assume that change is OK?
Thanks
Dave