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). 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.
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.
+ 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.
+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.
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.
Bernd
