Re: [PATCH] Selftest framework (v7)

[David Malcolm <dmalcolm at redhat dot com>]

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