[PATCH 4b] diagnostic-show-locus.c changes: Insertions

[Jeff Law]

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