On Thu, 2015-10-29 at 22:49 -0600, Jeff Law wrote:
> 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.
Thanks. I broke print_line into two methods, and eliminated the
debugging bits; I didn't touch the signedness within
get_line_width_without_trailing_whitespace (since this signedness is
coming from input.c, as noted before).
I've committed the combination of 4a+4b+4c+cleanups to trunk as r229884
(having bootstrapped®rtested).
For reference, I'm attaching the diffs (relative to 4b) for the cleanups
mentioned above.
Dave
>From 221d5daa7c88d1776a8ea1dc7bd5b4b1bb460ce5 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalc...@redhat.com>
Date: Sat, 31 Oct 2015 04:38:15 -0400
Subject: [PATCH] Split layout::print_line into two methods.
gcc/ChangeLog:
* diagnostic-show-locus.c (class colorizer): Update comment to
reflect split of layout::print_line into layout::print_source_line
and layout::print_annotation_line.
(struct line_bounds): New.
(class layout): Update comment to reflect split of
layout::print_line.
(layout::print_line): Delete, in favor of...
(layout::print_source_line): ...this new method and...
(layout::print_annotation_line): ...this new method.
(diagnostic_show_locus): Update for split of layout::print_line
into layout::print_source_line and layout::print_annotation_line.
---
gcc/diagnostic-show-locus.c | 78 ++++++++++++++++++++++++++++++---------------
1 file changed, 52 insertions(+), 26 deletions(-)
diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c
index 6865209..97f2853 100644
--- a/gcc/diagnostic-show-locus.c
+++ b/gcc/diagnostic-show-locus.c
@@ -67,9 +67,10 @@ struct point_state
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. */
+ active, filtering out unnecessary changes. This allows
+ layout::print_source_line and layout::print_annotation_line
+ to simply request a colorization code for *every* character they print,
+ via this class, and have the filtering be done for them here. */
class colorizer
{
@@ -128,12 +129,21 @@ class layout_range
layout_point m_caret;
};
+/* A struct for use by layout::print_source_line for telling
+ layout::print_annotation_line the extents of the source line that
+ it printed, so that underlines can be clipped appropriately. */
+
+struct line_bounds
+{
+ int m_first_non_ws;
+ int m_last_non_ws;
+};
+
/* A class to control the overall layout when printing a diagnostic.
The layout is determined within the constructor.
- It is then printed by repeatedly calling the "print_line" method.
- Each such call can print two lines: one for the source line itself,
- and potentially an "annotation" line, containing carets/underlines.
+ It is then printed by repeatedly calling the "print_source_line"
+ and "print_annotation_line" methods.
We assume we have disjoint ranges. */
@@ -146,7 +156,8 @@ class layout
int get_first_line () const { return m_first_line; }
int get_last_line () const { return m_last_line; }
- void print_line (int row);
+ bool print_source_line (int row, line_bounds *lbounds_out);
+ void print_annotation_line (int row, const line_bounds lbounds);
private:
bool
@@ -477,32 +488,30 @@ layout::layout (diagnostic_context * context,
show_ruler (context, line_width, m_x_offset);
}
-/* Print text describing a line of source code.
- This typically prints two lines:
+/* Attempt to print line ROW of source code, potentially colorized at any
+ ranges.
+ Return true if the line was printed, populating *LBOUNDS_OUT.
+ Return false if the source line could not be read, leaving *LBOUNDS_OUT
+ untouched. */
- (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)
+bool
+layout::print_source_line (int row, line_bounds *lbounds_out)
{
int line_width;
const char *line = location_get_source_line (m_exploc.file, row,
&line_width);
if (!line)
- return;
+ return false;
line += m_x_offset;
m_colorizer.set_normal_text ();
- /* Step 1: print the source code line. */
+ /* We will stop printing the source line at any trailing
+ whitespace. */
+ line_width = get_line_width_without_trailing_whitespace (line,
+ line_width);
- /* We will stop printing at any trailing whitespace. */
- line_width
- = get_line_width_without_trailing_whitespace (line,
- line_width);
pp_space (m_pp);
int first_non_ws = INT_MAX;
int last_non_ws = 0;
@@ -547,10 +556,19 @@ layout::print_line (int row)
}
pp_newline (m_pp);
- /* Step 2: print a line consisting of the caret/underlines for the
- given source line. */
+ lbounds_out->m_first_non_ws = first_non_ws;
+ lbounds_out->m_last_non_ws = last_non_ws;
+ return true;
+}
+
+/* Print a line consisting of the caret/underlines for the given
+ source line. */
+
+void
+layout::print_annotation_line (int row, const line_bounds lbounds)
+{
int x_bound = get_x_bound_for_row (row, m_exploc.column,
- last_non_ws);
+ lbounds.m_last_non_ws);
pp_space (m_pp);
for (int column = 1 + m_x_offset; column < x_bound; column++)
@@ -558,7 +576,8 @@ layout::print_line (int row)
bool in_range_p;
point_state state;
in_range_p = get_state_at_point (row, column,
- first_non_ws, last_non_ws,
+ lbounds.m_first_non_ws,
+ lbounds.m_last_non_ws,
&state);
if (in_range_p)
{
@@ -734,7 +753,14 @@ diagnostic_show_locus (diagnostic_context * context,
for (int row = layout.get_first_line ();
row <= last_line;
row++)
- layout.print_line (row);
+ {
+ /* Print the source line, followed by an annotation line
+ consisting of any caret/underlines. If the source line can't
+ be read, print nothing. */
+ line_bounds lbounds;
+ if (layout.print_source_line (row, &lbounds))
+ layout.print_annotation_line (row, lbounds);
+ }
/* The closing scope here leads to the dtor for layout and thus
colorizer being called here, which affects the precise
--
1.8.5.3
>From 4b1113880023d404ebb52d7efab934b913ff3284 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalc...@redhat.com>
Date: Mon, 2 Nov 2015 15:35:34 -0500
Subject: [PATCH] Fix nits in diagnostic-show-locus.c
gcc/ChangeLog:
* diagnostic-show-locus.c (layout::layout): Eliminate call to
show_ruler.
(layout::get_state_at_point): Eliminate debug code.
(show_ruler): Delete.
---
gcc/diagnostic-show-locus.c | 51 ---------------------------------------------
1 file changed, 51 deletions(-)
diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c
index 97f2853..22203cd 100644
--- a/gcc/diagnostic-show-locus.c
+++ b/gcc/diagnostic-show-locus.c
@@ -36,9 +36,6 @@ along with GCC; see the file COPYING3. If not see
# include <sys/ioctl.h>
#endif
-static void
-show_ruler (diagnostic_context *context, int max_width, int x_offset);
-
/* Classes for rendering source code and diagnostics, within an
anonymous namespace.
The work is done by "class layout", which embeds and uses
@@ -483,9 +480,6 @@ layout::layout (diagnostic_context * context,
m_x_offset = column - right_margin;
gcc_assert (m_x_offset >= 0);
}
-
- if (0)
- show_ruler (context, line_width, m_x_offset);
}
/* Attempt to print line ROW of source code, potentially colorized at any
@@ -615,17 +609,6 @@ layout::get_state_at_point (/* Inputs. */
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");
-
if (range->contains_point (row, column))
{
out_state->range_idx = i;
@@ -694,40 +677,6 @@ layout::get_x_bound_for_row (int row, int caret_column,
} /* End of anonymous namespace. */
-/* 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)
-{
- /* Hundreds. */
- if (max_width > 99)
- {
- pp_space (context->printer);
- for (int column = 1 + x_offset; column < max_width; column++)
- if (0 == column % 10)
- pp_character (context->printer, '0' + (column / 100) % 10);
- else
- pp_space (context->printer);
- pp_newline (context->printer);
- }
-
- /* Tens. */
- pp_space (context->printer);
- for (int column = 1 + x_offset; column < max_width; column++)
- if (0 == column % 10)
- pp_character (context->printer, '0' + (column / 10) % 10);
- else
- pp_space (context->printer);
- pp_newline (context->printer);
-
- /* Units. */
- pp_space (context->printer);
- for (int column = 1 + x_offset; column < max_width; column++)
- pp_character (context->printer, '0' + (column % 10));
- pp_newline (context->printer);
-}
-
/* Print the physical source code corresponding to the location of
this diagnostic, with additional annotations. */
--
1.8.5.3
