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
