Hello Manuel,
Sorry for taking so long to reply to this.
FWIW, I like the direction of this. I find fix-it hints cool in
general. So thank you for working on this.
Manuel López-Ibáñez a écrit:
> This patch implements fix-it hints. See https://gcc.gnu.org/PR62314
>
> When the caret line is active (which is the default), this adds an
> additional source-line indicating how to fix the code:
>
> gcc/testsuite/g++.dg/template/crash83.C:5:21: error: an explicit
> specialization must be preceded by 'template <>'
> template: > struct B {}; // { dg-error
> "explicit specialization|expected" }
> ^
> template<>
It looks like your mail user agent wrapped a line above, making it hard
to read. I suspect it should have been:
template > struct B {}; // { dg-error "explicit
specialization|expected" }
^
template<>
> When the caret line is disabled with -fno-diagnostics-show-caret, the
> fix-it hint is printed as:
>
> gcc/testsuite/g++.dg/template/crash83.C:5:21: error: an explicit
> specialization must be preceded by 'template <>'
> gcc/testsuite/g++.dg/template/crash83.C:5:21: fixit: template<>
>
> The latter form may allow an IDE (such as emacs) to automatically
> apply the fix.
Nice. Is the "fixit:" prefix used by other compilers too? Or are there
variations from compiler to compiler?
> Currently, fix-it hints are limited to insertions at one single
> location, whereas Clang allows insertions, deletions, and replacements
> at arbitrary location ranges.
Do you have example of each of these kinds of fix-it hints? (deletions,
replacement at location ranges). I think it'd be nice to have an idea
of what needs to be done, even if we are not doing it "in extenso" right
now.
> Opinions? Is the proposed interface/implementation acceptable?
Please read my comments below.
> Any other diagnostics that could use a fix-it hint? In principle, we
> should only give them when we are sure that the proposed fix will fix
> the error or silence a warning. For example, the C++ parser often
> says 'x' expected before 'y' but adding x before y rarely fixes
> anything.
I am thinking that maybe the diagnostic about the missing ";" after a
struct/class declaration might be a candidate for this fix-it hint
feature.
It's emitted by cp_parser_class_specifier_1() at:
if (CLASSTYPE_DECLARED_CLASS (type))
error_at (loc, "expected %<;%> after class definition");
else if (TREE_CODE (type) == RECORD_TYPE)
error_at (loc, "expected %<;%> after struct definition");
else if (TREE_CODE (type) == UNION_TYPE)
error_at (loc, "expected %<;%> after union definition");
else
gcc_unreachable ();
[...]
> +
> +static int
> +adjust_column (int line_width, int max_width, int column)
Missing comments for this function.
[...]
> +static const char *
> +get_source_line_and_column (location_t loc, int *line_width, int *column)
> +{
Likewise.
[...]
> /* Print the physical source line corresponding to the location of
> this diagnostic, and a caret indicating the precise column. */
> void
> diagnostic_show_locus (diagnostic_context * context,
> const diagnostic_info *diagnostic)
> {
[...]
>context->last_location = diagnostic->location;
> - s = expand_location_to_spelling_point (diagnostic->location);
> - line = location_get_source_line (s, &line_width);
> - if (line == NULL || s.column > line_width)
> + line = get_source_line_and_column (diagnostic->location,
> + &line_width, &column);
> + if (line == NULL)
> return;
>
>max_width = context->caret_max_width;
> - line = adjust_line (line, line_width, max_width, &(s.column));
> + line = adjust_line (line, line_width, max_width, &column);
Apparently, each time we call get_source_line_and_column, we also call
adjust_line on it. So maybe we want to have a
get_adjusted_source_line_and_column (or something like that) that does
it all?
[...]
> @@ -325,13 +345,13 @@ diagnostic_show_locus (diagnostic_contex
>pp_newline (context->printer);
>caret_cs = colorize_start (pp_show_color (context->printer), "caret");
>caret_ce = colorize_stop (pp_show_color (context->printer));
>
>/* pp_printf does not implement %*c. */
> - size_t len = s.column + 3 + strlen (caret_cs) + strlen (caret_ce);
> + size_t len = column + 3 + strlen (caret_cs) + strlen (caret_ce);
>buffer = XALLOCAVEC (char, len);
> - snprintf (buffer, len, "%s %*c%s", caret_cs, s.column, context->caret_char,
> + snprintf (buffer, len, "%s %*c%s", caret_cs, column, context->caret_char,
> caret_ce);
Maybe you should factorize out the printing of a colored line starting
at given a column, rather than copy-pasting this in fixit_hint() later?
[...]
>diagnostic_set_info (&diagnostic, gmsgid, &ap, location, DK_NOTE);
>report_diagnostic (&diagnostic);
>va_end (ap
