From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 118350 invoked by alias); 30 Oct 2015 04:49:19 -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 118340 invoked by uid 89); 30 Oct 2015 04:49:18 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 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; Fri, 30 Oct 2015 04:49:17 +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 (Postfix) with ESMTPS id 71CCE8E233 for ; Fri, 30 Oct 2015 04:49:16 +0000 (UTC) Received: from localhost.localdomain (ovpn-113-196.phx2.redhat.com [10.3.113.196]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t9U4nGFx019257; Fri, 30 Oct 2015 00:49:16 -0400 Subject: Re: [PATCH 4b] diagnostic-show-locus.c changes: Insertions To: David Malcolm References: <56300286.4040103@redhat.com> <1446055764-23753-1-git-send-email-dmalcolm@redhat.com> <1446055764-23753-3-git-send-email-dmalcolm@redhat.com> Cc: gcc-patches@gcc.gnu.org From: Jeff Law Message-ID: <5632F6CB.7090408@redhat.com> Date: Fri, 30 Oct 2015 04:53:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <1446055764-23753-3-git-send-email-dmalcolm@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-10/txt/msg03300.txt.bz2 On 10/28/2015 12:09 PM, David Malcolm wrote: > gcc/ChangeLog: > * diagnostic-show-locus.c (struct point_state): New struct. > (class colorizer): New class. > (class layout_point): New class. > (class layout_range): New class. > (class layout): New class. > (colorizer::colorizer): New ctor. > (colorizer::~colorizer): New dtor. > (layout::layout): New ctor. > (layout::print_line): New method. > (layout::get_state_at_point): New method. > (layout::get_x_bound_for_row): New method. > (show_ruler): New function. > (diagnostic_show_locus): Reimplement in terms of class layout. > --- > +}; > + > +/* A class to inject colorization codes when printing the diagnostic locus. > + > + It has one kind of colorization for each of: > + - normal text > + - range 0 (the "primary location") > + - range 1 > + - range 2 > + > + The class caches the lookup of the color codes for the above. > + > + The class also has responsibility for tracking which of the above is > + active, filtering out unnecessary changes. This allows layout::print_line > + to simply request a colorization code for *every* character it prints > + through this class, and have the filtering be done for it here. */ Not asking you to do anything here -- hopefully this isn't a huge burden on the diagnostic performance. Normally I wouldn't even notice except that we're inserting colorization on every character. That kind of model can get expensive. Something to watch out for -- though I doubt we do he massive diagnostic spews we used to which is probably the only place it'd be noticeable. > + > +/* A point within a layout_range; similar to an expanded_location, > + but after filtering on file. */ > + > +class layout_point > +{ > + public: > + layout_point (const expanded_location &exploc) > + : m_line (exploc.line), > + m_column (exploc.column) {} > + > + int m_line; > + int m_column; > +}; Is this even deserving of its own class? If you pulled up m_line/m_column you don't need the class, though I guess you need thee of each, one for the start, one for the finish & one for the caret, which in turn bloats the layout_range's constructor. So I guess this is OK. > +/* Given a source line LINE of length LINE_WIDTH, determine the width > + without any trailing whitespace. */ > + > +static int > +get_line_width_without_trailing_whitespace (const char *line, int line_width) > +{ > + int result = line_width; > + while (result > 0) > + { > + char ch = line[result - 1]; > + if (ch == ' ' || ch == '\t') > + result--; > + else > + break; > + } > + gcc_assert (result >= 0); > + gcc_assert (result <= line_width); > + gcc_assert (result == 0 || > + (line[result - 1] != ' ' > + && line[result -1] != '\t')); > + return result; > +} If you use an unsigned for the line width, don't all the asserts become redundant & unnecessary? I love the sanity checking and I could see how it might be useful it someone were to reimplmenent this function at a later date. So maybe keep. > + > +/* Implementation of class layout. */ > + > +/* Constructor for class layout. > + > + Filter the ranges from the rich_location to those that we can > + sanely print, populating m_layout_ranges. > + Determine the range of lines that we will print. > + Determine m_x_offset, to ensure that the primary caret > + will fit within the max_width provided by the diagnostic_context. */ > + > +layout::layout (diagnostic_context * context, > + const diagnostic_info *diagnostic) [ ... ] > + if (0) > + show_ruler (context, line_width, m_x_offset); Debugging code? If it's if (0) you should probably delete it at this point. > +} > + > +/* Print text describing a line of source code. > + This typically prints two lines: > + > + (1) the source code itself, potentially colorized at any ranges, and > + (2) an annotation line containing any carets/underlines > + describing the ranges. */ > + > +void > +layout::print_line (int row) Consider breaking this into two functions. One to print the source line and another to print caret/underlines. + > +/* Return true if (ROW/COLUMN) is within a range of the layout. > + If it returns true, OUT_STATE is written to, with the > + range index, and whether we should draw the caret at > + (ROW/COLUMN) (as opposed to an underline). */ > + > +bool > +layout::get_state_at_point (/* Inputs. */ > + int row, int column, > + int first_non_ws, int last_non_ws, > + /* Outputs. */ > + point_state *out_state) > +{ > + layout_range *range; > + int i; > + FOR_EACH_VEC_ELT (m_layout_ranges, i, range) > + { > + if (0) > + fprintf (stderr, > + "range ( (%i, %i), (%i, %i))->contains_point (%i, %i): %s\n", > + range->m_start.m_line, > + range->m_start.m_column, > + range->m_finish.m_line, > + range->m_finish.m_column, > + row, > + column, > + range->contains_point (row, column) ? "true" : "false"); More old debugging code that needs to be removed? > + > +/* For debugging layout issues in diagnostic_show_locus and friends, > + render a ruler giving column numbers (after the 1-column indent). */ > + > +static void > +show_ruler (diagnostic_context *context, int max_width, int x_offset) Seems like it ought to be DEBUG_FUNCTION or removed. I believe it's only caller is in if (0) code in layout's ctor. Overall this looks good. Take the actions you deem appropriate WRT the debugging bits, breaking print_line into two functions and the signed vs unsigned stuff in get_line_width_without_trailing_whitespace and it's good for the trunk. Jeff