Hello David,
I like this! Thank you very much for working on this.
I think this patch is in great shape, and once we agree on some of the
nits I have commented on below, I think it should go in. Oh, it also
needs the first patch (1/5, dejagnu first) to go in first, as this one
depends on it. I defer to the dejagnu maintainers for that one.
The line-map parts are OK to me too, but I have no power on those. So I
defer to the FE maintainers for that. The diagnostics parts of the
Fortran, C++ and C FE look good to me too; these are just well contained
mechanical adjustments, but I defer to FE maintainers for the final
word.
Please find my comments below.
[...]
diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c
[...]
+/* A class to inject colorization codes when printing the diagnostic locus,
+ tracking state as it goes. */
+
+class colorizer
+{
[...]
+ void set_state (int state);
+ void begin_state (int state);
+ void finish_state (int state);
I think the concept of state could use a little bit of explanation, at
least to say that there are the same number of states, as the number
of ranges. And that the 'state' argument to these functions really is
the range index.
Also, I am thinking that there should maybe be a layout::state type,
which would have two notional properties (for now): range_index and
draw_caret_p. So that this function:
+bool
+layout::get_state_at_point (/* Inputs. */
+ int row, int column,
+ int first_non_ws, int last_non_ws,
+ /* Outputs. */
+ int *out_range_idx,
+ bool *out_draw_caret_p)
Would take just one output parameter, e.g, a reference to
layout::state.
+layout::layout (diagnostic_context * context,
+ const diagnostic_info *diagnostic)
[...]
+ if (loc_range->m_finish.file != m_exploc.file)
+ continue;
+ if (loc_range->m_show_caret_p)
+ if (loc_range->m_finish.file != m_exploc.file)
+ continue;
I think the second if clause is redundant.
+ if (0)
+ show_ruler (context, line_width, m_x_offset);
This should probably be removed from the final code to be committed.
[...]
+/* Get the column beyond the rightmost one that could contain a caret or
+ range marker, given that we stop rendering at trailing whitespace. */
+
+int
+layout::get_x_bound_for_row (int row, int caret_column,
+ int last_non_ws)
Please describe what the parameters mean here, especially last_non_ws.
I had to read its code to know that last_non_ws was the *column* of
the last non white space character.
[...]
+void
+layout::print_line (int row)
This function is neat. I like it! :-)
[...]
void
diagnostic_show_locus (diagnostic_context * context,
const diagnostic_info *diagnostic)
@@ -75,16 +710,25 @@ diagnostic_show_locus (diagnostic_context * context,
return;
+ /* The GCC 5 routine. */
I'd say the GCC <= 5 routine ;-)
+ else
+ /* The GCC 6 routine. */
And here, the GCC > 5 routine.
I would be surprised to see this patch in particular incur any
noticeable increase in time and space consumption, but, have you noticed
anythying related to that during bootstrap?
Cheers,
--
Dodji
