From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11010 invoked by alias); 25 Sep 2014 09:34:11 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 10998 invoked by uid 89); 25 Sep 2014 09:34:10 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.1 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 25 Sep 2014 09:34:09 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s8P9Y72L009251 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Thu, 25 Sep 2014 05:34:07 -0400 Received: from localhost (ovpn-116-61.ams2.redhat.com [10.36.116.61]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s8P9Y6dK006227 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Thu, 25 Sep 2014 05:34:07 -0400 Received: by localhost (Postfix, from userid 1000) id 2AF721658C1; Thu, 25 Sep 2014 11:34:05 +0200 (CEST) From: Dodji Seketeli To: Manuel =?utf-8?B?TMOzcGV6LUliw6HDsWV6?= Cc: Gcc Patch List , Jason Merrill Subject: Re: [RFC/PATCH] Fix-it hints References: X-URL: http://www.seketeli.net/~dodji Date: Thu, 25 Sep 2014 09:34:00 -0000 In-Reply-To: ("Manuel \=\?utf-8\?B\?TMOzcGV6LUliw6HDsWV6Iidz\?\= message of "Sun, 31 Aug 2014 23:57:25 +0200") Message-ID: <87d2akqc1e.fsf@seketeli.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SW-Source: 2014-09/txt/msg02234.txt.bz2 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=C3=B3pez-Ib=C3=A1=C3=B1ez a =C3=A9crit: > 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 s= pecialization|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) =3D=3D RECORD_TYPE) error_at (loc, "expected %<;%> after struct definition"); else if (TREE_CODE (type) =3D=3D 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 =3D diagnostic->location; > - s =3D expand_location_to_spelling_point (diagnostic->location); > - line =3D location_get_source_line (s, &line_width); > - if (line =3D=3D NULL || s.column > line_width) > + line =3D get_source_line_and_column (diagnostic->location, > + &line_width, &column); > + if (line =3D=3D NULL) > return; >=20=20 > max_width =3D context->caret_max_width; > - line =3D adjust_line (line, line_width, max_width, &(s.column)); > + line =3D 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 =3D colorize_start (pp_show_color (context->printer), "caret"= ); > caret_ce =3D colorize_stop (pp_show_color (context->printer)); >=20=20 > /* pp_printf does not implement %*c. */ > - size_t len =3D s.column + 3 + strlen (caret_cs) + strlen (caret_ce); > + size_t len =3D column + 3 + strlen (caret_cs) + strlen (caret_ce); > buffer =3D 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_ch= ar, > 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); > } >=20=20 > +/* 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, --=20 Dodji