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 <lopeziba...@gmail.com> 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<typename = class A<0>: > 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<typename = class A<0> > 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); > } > > +/* A fix-it hint at LOCATION. Use this recommended textual > + replacements after another diagnostic message. */ > +void > +fixit_hint (location_t location, const char *msg, ...) I would call this "emit_fixit_hint" to make its intend clearer. Also, I'd be more extensive in the comment of this function. Maybe give the introductory example you gave for this patch? Also, just curious, do you have an idea about how you would extend this to support deletion and replacement fix-it hints? For instance, from a user (of the diagnostics primitives) code standpoint, would these be done through additional functions like emit_deletion_fixit() and emit_replacement_fixit() with possible different parameters? Or would you rather add additional parameters to emit_fixing_hint() to support that? > +{ > + diagnostic_info diagnostic; > + va_list ap; > + > + va_start (ap, msg); > + diagnostic_set_info (&diagnostic, msg, &ap, location, DK_FIXIT); > + if (!global_dc->show_caret) > + { > + report_diagnostic (&diagnostic); > + } Useless curly brackets here. Thank you for looking into this. Cheers, -- Dodji
