From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 101978 invoked by alias); 13 Nov 2018 00:56:41 -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 101963 invoked by uid 89); 13 Nov 2018 00:56:41 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=HTo:D*mathworks.com, H*Ad:D*mathworks.com 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 ESMTP; Tue, 13 Nov 2018 00:56:39 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 549F987628; Tue, 13 Nov 2018 00:56:38 +0000 (UTC) Received: from ovpn-116-30.phx2.redhat.com (ovpn-116-30.phx2.redhat.com [10.3.116.30]) by smtp.corp.redhat.com (Postfix) with ESMTP id B814D608FA; Tue, 13 Nov 2018 00:56:37 +0000 (UTC) Message-ID: <1542070596.4619.23.camel@redhat.com> Subject: Re: [PATCH v3 3/3] PR preprocessor/83173: Enhance -fdump-internal-locations output From: David Malcolm To: Mike Gulick , "gcc-patches@gcc.gnu.org" Date: Tue, 13 Nov 2018 00:56:00 -0000 In-Reply-To: <3a5fbdbc-2095-e4b2-1489-e4577bd6c6e8@mathworks.com> References: <7d281f10-a4e0-d81e-c405-d77ceda86f5b@mathworks.com> <20181101155607.11388-1-mgulick@mathworks.com> <20181101155607.11388-4-mgulick@mathworks.com> <1541192656.14521.336.camel@redhat.com> <3a5fbdbc-2095-e4b2-1489-e4577bd6c6e8@mathworks.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-11/txt/msg01009.txt.bz2 On Mon, 2018-11-12 at 21:13 +0000, Mike Gulick wrote: > On 11/2/18 5:04 PM, David Malcolm wrote: > > On Thu, 2018-11-01 at 11:56 -0400, Mike Gulick wrote: > > > 2017-10-31 Mike Gulick > > > > > > PR preprocessor/83173 > > > * gcc/input.c (dump_location_info): Dump reason and > > > included_from fields from line_map_ordinary struct. Fix > > > indentation when location > 5 digits. > > > > > > * libcpp/location-example.txt: Update example > > > -fdump-internal-locations output. > > > --- > > > gcc/input.c | 49 +++++- > > > libcpp/location-example.txt | 333 +++++++++++++++++++++--------- > > > ---- > > > -- > > > 2 files changed, 241 insertions(+), 141 deletions(-) > > > > Sorry about the belated response. This is a nice enhancement; some > > nits below. > > > > > diff --git a/gcc/input.c b/gcc/input.c > > > index a94a010f353..f938a37f20e 100644 > > > --- a/gcc/input.c > > > +++ b/gcc/input.c > > > @@ -1075,6 +1075,17 @@ dump_labelled_location_range (FILE > > > *stream, > > > fprintf (stream, "\n"); > > > } > > > > > > +#define NUM_DIGITS(x) ((x) >= 1000000000 ? 10 : \ > > > + (x) >= 100000000 ? 9 : \ > > > + (x) >= 10000000 ? 8 : \ > > > + (x) >= 1000000 ? 7 : \ > > > + (x) >= 100000 ? 6 : \ > > > + (x) >= 10000 ? 5 : \ > > > + (x) >= 1000 ? 4 : \ > > > + (x) >= 100 ? 3 : \ > > > + (x) >= 10 ? 2 : \ > > > + 1) > > > > diagnostic-show-locus.c has a function "num_digits" (currently > > static) > > and, fwiw, a unit test. It would be good to share the > > implementation. > > > > I initially tried to use this function by just adding "extern int > num_digits(int);" into diagnostic-core.h, but that failed to link, so > it seems > like diagnostic-show-locus.c is not included in whatever library > input.c gets > linked with (I forget which library it was trying to link). Both input.o and diagnostic-show-locus.o are in OBJS-libcommon, so I'm not sure what went wrong. > Instead I moved > num_digits and its unit test to diagnostic.c, and added the extern > definition to > diagnostic-core.h. That builds and tests successfully. Does that > seem like a > reasonable way to do this? Thanks. That sounds good (maybe put the decl in diagnostic.h rather than diagnostic-core.h; the latter is used in lots of places, whereas the former is more about implementation details). > > > /* Write a visualization of the locations in the line_table to > > > STREAM. */ > > > > > > void > > > @@ -1104,6 +1115,35 @@ dump_location_info (FILE *stream) > > > map->m_column_and_range_bits - map- > > > >m_range_bits); > > > fprintf (stream, " range bits: %i\n", > > > map->m_range_bits); > > > + const char * reason; > > > + switch (map->reason) { > > > + case LC_ENTER: > > > + reason = "LC_ENTER"; > > > + break; > > > + case LC_LEAVE: > > > + reason = "LC_LEAVE"; > > > + break; > > > + case LC_RENAME: > > > + reason = "LC_RENAME"; > > > + break; > > > + case LC_RENAME_VERBATIM: > > > + reason = "LC_RENAME_VERBATIM"; > > > + break; > > > + case LC_ENTER_MACRO: > > > + reason = "LC_RENAME_MACRO"; > > > + break; > > > + default: > > > + reason = "Unknown"; > > > + } > > > + fprintf (stream, " reason: %d (%s)\n", map->reason, > > > reason); > > > + > > > + const line_map_ordinary *includer_map > > > + = linemap_included_from_linemap (line_table, map); > > > + fprintf (stream, " included from map: %d\n", > > > + includer_map ? int (includer_map - line_table- > > > > info_ordinary.maps) > > > > > > + : -1); > > > > I'm not a fan of "-1" here; it's a NULL pointer in the original > > data. > > How about "n/a" for that case? > > > > That's a good suggestion. Thanks. > > > > + fprintf (stream, " included from location: %d\n", > > > + linemap_included_from (map)); > > > > ...or merging it with this line, for something like: > > > > included from location: 127 (in ordinary map 2) > > > > vs: > > > > included from location: 0 > > > > [...snip...] > > > > Other than that, this is OK for trunk, assuming your contributor > > paperwork is in place. > > > > Dave > > > > What is the preferred way to re-send this patch? Should I re-send > the entire > patch series as v4, or just an updated version of this single patch? The latter: just an updated version of the changed patch. IIRC the rest is all approved. > > Also, I'm waiting on FSF for assignment paperwork. I've re-pinged > them after > waiting a week. Thanks. > Thanks for the feedback and help. > > -Mike