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.
Maybe the comment for the class is unclear: we're *not* emitting color
codes on every character: in fact preventing that is the point of the
colorizer class. The layout class does indeed request a color code for
each character, but the colorizer class filters them so that the only
actual colorization codes that are emitted are when the status changes
(and they're not emitted at all if -fdiagnostics-color=never).
It's a separation of responsibilities, to keep the layout class simpler.
[it's even tested, in that
gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-color.c
only contains status-change color codes in the
dg-begin/end-multiline-output directives]
> > +
> > +/* 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.
Yes, it's to simplify layout_range.
> > +/* 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.
FWIW, the type for the line width ultimately comes from the 3rd param to
location_get_source_line:
extern const char *location_get_source_line (const char *file_path, int line,
int *line_size);
We could change that, since negative values are clearly bogus, but to do
seems to me to be tangential to this patch kit.
> > +
> > +/* 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.
There's some state set up when printing the source line that's used when
printing the annotation line, so it's not trivial to break them,
although desirable; I may introduce a
class line_layout
to encapsulate that, if that sounds reasonable.
> +
> > +/* 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?
Will remove.
> > +
> > +/* 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.
Will remove.
> 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
Thanks
Dave
