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
