From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 56615 invoked by alias); 7 May 2015 20:16:09 -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 56590 invoked by uid 89); 7 May 2015 20:16:08 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.7 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients 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, 07 May 2015 20:16:07 +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 t47KG4lm018956 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Thu, 7 May 2015 16:16:05 -0400 Received: from localhost (ovpn-116-48.ams2.redhat.com [10.36.116.48]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t47KG1ns010187 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Thu, 7 May 2015 16:16:03 -0400 Received: by localhost (Postfix, from userid 1001) id 2E9DB1566; Thu, 7 May 2015 22:16:00 +0200 (CEST) From: Dodji Seketeli To: Manuel =?utf-8?B?TMOzcGV6LUliw6HDsWV6?= Cc: Gcc Patch List , Tobias Burnus , "fortran\@gcc.gnu.org List" Subject: Re: [PATCH diagnostics/fortran] Handle two locations for the same diagnostic. Convert all gfc_warning_1 and gfc_notify_std_1 calls References: X-URL: http://www.redhat.com Date: Thu, 07 May 2015 20:16:00 -0000 In-Reply-To: ("Manuel \=\?utf-8\?B\?TMOzcGV6LUliw6HDsWV6Iidz\?\= message of "Mon, 20 Apr 2015 22:00:43 +0200") Message-ID: <86h9rogn3z.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SW-Source: 2015-05/txt/msg00589.txt.bz2 Hello Manuel, Sorry for my late reply, and thank you very much for working on this. I have looked at the patch and I like it! I guess I just have some few lateral nits to pick. > The Fortran FE allows diagnostics with two different locations. > Depending on whether these locations are on the same line or not, this > may produce one or two caret lines. This is the last remaining issue > left to make Fortran diagnostics use the common code. Yes, I remember. [...] > I added support for this in the common diagnostics, although Fortran > is the only user for now. The new common code should be flexible > enough to support the Clang style (which I guess is likely to be what > C/C++ FEs end up supporting sooner or later) while still supporting > the Fortran style. Good. > Supporting this in the common diagnostics code requires having two > locations in struct diagnostic_info and two pointers in struct > text_info. That seems a waste and overtly complex. Thus, I moved the > new location array directly to struct text_info and added an accessor > function diagnostic_location(). Agreed. [...] > Index: gcc/pretty-print.h [...] > --- gcc/pretty-print.h (revision 222087) > +++ gcc/pretty-print.h (working copy) > @@ -33,11 +33,13 @@ along with GCC; see the file COPYING3.=20=20 > struct text_info > { > const char *format_spec; > va_list *args_ptr; > int err_no; /* for %m */ > - location_t *locus; > + /* This message can have associated two locations at most. If the > + first location is UNKNOWN_LOCATION, the second is not valid. */ > + location_t location[2]; Here, I would call the data member locations (note the 's' at the end). Also, I'd define a constant (a macro, sigh) named like e.g, MAX_LOCATIONS_PER_MESSAGE that is set to '2', rather than carrying forcing users of these locations to know that there are specifically two locations here. [...] > void **x_data; > }; >=20=20 > Index: gcc/diagnostic.h [..] > /* A diagnostic is described by the MESSAGE to send, the FILE and LINE of > its context and its KIND (ice, error, warning, note, ...) See comple= te > list in diagnostic.def. */ > struct diagnostic_info > { > + /* Text to be formatted. It also contains the location(s) for this > + diagnostic. */ > text_info message; > - location_t location; > unsigned int override_column; [...] >=20=20 > - /* Character used for caret diagnostics. */ > - char caret_char; > + /* Characters used for caret diagnostics. */ > + char caret_char[2]; >=20=20 Here, I'd call the data member caret_chars (with an 's' at the end) and I'd use the same MAX_LOCATIONS_PER_MESSAGE constant as above, rather that the '2' literal. [...] > --- gcc/tree-pretty-print.c (revision 222087) > +++ gcc/tree-pretty-print.c (working copy) [...] > @@ -3618,12 +3618,11 @@ newline_and_indent (pretty_printer *pp,=20 >=20=20 > void > percent_K_format (text_info *text) > { > tree t =3D va_arg (*text->args_ptr, tree), block; > - gcc_assert (text->locus !=3D NULL); > - *text->locus =3D EXPR_LOCATION (t); > + text->location[0] =3D EXPR_LOCATION (t); I guess I'd prefer to have an accessor (e.g, source_location text_info_location(text_info, int index_of_location)) that returns the location and checks that we are accessing a location that is below the MAX_LOCATIONS_PER_MESSAGE maximum, rather than just doing text->locations[0] here. And, likewise for the other similar spots that access text_info::locations. [...] > --- gcc/diagnostic.c (revision 222087) > +++ gcc/diagnostic.c (working copy) > @@ -144,11 +144,12 @@ diagnostic_initialize (diagnostic_contex [...] > context->show_caret =3D false; > diagnostic_set_caret_max_width (context, pp_line_cutoff (context->prin= ter)); > - context->caret_char =3D '^'; > + context->caret_char[0] =3D '^'; > + context->caret_char[1] =3D '^'; I'd use a loop from O to MAX_LOCATIONS_PER_MESSAGE to initialize this. Or maybe rather a dedicated little function for this even; as you see fit. [...] > @@ -239,11 +240,12 @@ diagnostic_set_info_translated (diagnost > diagnostic_t kind) > { > diagnostic->message.err_no =3D errno; > diagnostic->message.args_ptr =3D args; > diagnostic->message.format_spec =3D msg; > - diagnostic->location =3D location; > + diagnostic->message.location[0] =3D location; > + diagnostic->message.location[1] =3D UNKNOWN_LOCATION; I'd use a loop from 1 to to MAX_LOCATIONS_PER_MESSAGE to set the UNKNOWN_LOCATION. I understand that the loop will step only one iteration, but the goal is to be ready for when a front end is going to need three or more locations per messages. We'd then just need to to adjust MAX_LOCATIONS_PER_MESSAGE and the whole thing would "almost" Just Work =E2=84=A2. [...] > /* Print the physical source line corresponding to the location of > - this diagnostic, and a caret indicating the precise column. */ > + this diagnostic, and a caret indicating the precise column. This > + function only prints two caret characters if the two locations given = by > + DIAGNOSTIC are on the same locus according to I am confused by what you mean by "same locus" here. Do you mean the "same line" ? If yes, then maybe the other spots where you talk about "same locus" should be updated too. More on this later below. > + diagnostic_same_locus(). */ > void > diagnostic_show_locus (diagnostic_context * context, > const diagnostic_info *diagnostic) > { > - const char *line; > - int line_width; > - char *buffer; > - expanded_location s; > - int max_width; > - const char *saved_prefix; > - const char *caret_cs, *caret_ce; > + expanded_location s1, s2; Here, I'd initialize s1 and s2. Well, at least s2. So that ... > if (!context->show_caret > - || diagnostic->location <=3D BUILTINS_LOCATION > - || diagnostic->location =3D=3D context->last_location) > + || diagnostic_location (diagnostic, 0) <=3D BUILTINS_LOCATION > + || diagnostic_location (diagnostic, 0) =3D=3D context->last_locati= on) > return; >=20=20 > - context->last_location =3D diagnostic->location; > - s =3D diagnostic_expand_location (diagnostic); > - line =3D location_get_source_line (s, &line_width); > - if (line =3D=3D NULL || s.column > line_width) > + context->last_location =3D diagnostic_location (diagnostic, 0); > + s1 =3D diagnostic_expand_location (diagnostic, 0); > + if (diagnostic_location (diagnostic, 1) > BUILTINS_LOCATION) > + s2 =3D diagnostic_expand_location (diagnostic, 1); > + else > + s2.column =3D 0; ... we can do away with the else branch here. [...] I would add a comment to this new function here: > -/* Expand the location of this diagnostic. Use this function for consist= ency. */ > +static inline location_t > +diagnostic_location (const diagnostic_info * diagnostic, int which =3D 0) > +{ > + return diagnostic->message.location[which]; > +} > + > + > +/* Expand the location of this diagnostic. Use this function for consist= ency. > + WHICH specifies which location. By default, expand the first one. */ [...] >=20=20 > +/* Return true if the two locations can be represented within the same > + locus. This is used not only for the prefix but also to determine > + whether to print one or two caret lines. */ > + > +static inline bool > +diagnostic_same_locus (const diagnostic_context *context, > + expanded_location s1, expanded_location s2) > +{ > + return s2.column && s1.line =3D=3D s2.line=20 > + && context->caret_max_width - 10 > abs (s1.column - s2.column); > +} My understanding is that this function checks that the two locations s1 and s2 are on the same line and are "close enough"; that is, they fit into the maximum "display width" that the diagnostic machinery allows for a line. That display width is, from I see in adjust_line(), diagnostic_context::caret_max_width minus a margin; and the margin is 10. If I am right, then I think the name diagnostic_same_locus() should be changed to something more meaningful. The 'locus' here seems confusing and makes the code hard to understand, IMHO. Especially given that we are talking about 'locations' as well. What do you think? I would also change the 10 literal into a named constant and use it at the other spots where we use the 10 today. Because otherwise, seeing that 10 literal magically appear in this function like this is ... surprising. Is it not? [...] > Bootstrapped and regression tested on x86_64-linux-gnu. Thanks! >=20 > OK? >=20 It's mostly OK to me, barring the points I have raised and for which I need input from you. Sorry again for taking so much time in reviewing this. Thanks! --=20 Dodji