[committed] diagnostics: support compact printing of secondary locations
David Malcolm
dmalcolm@redhat.com
Tue Jul 11 14:09:00 GMT 2017
On Mon, 2017-07-03 at 19:57 +0100, Richard Sandiford wrote:
> [Thanks for all your diagnostic work btw.]
>
> David Malcolm <dmalcolm@redhat.com> writes:
> > clang can also print notes about matching opening symbols
> > e.g. the note here:
> >
> > missing-symbol-2.c:25:22: error: expected ']'
> > const char test [42;
> > ^
> > missing-symbol-2.c:25:19: note: to match this '['
> > const char test [42;
> > ^
> > which, although somewhat redundant for this example, seems much
> > more
> > useful if there's non-trivial nesting of constructs, or more than a
> > few
> > lines separating the open/close symbols (e.g. showing a stray
> > "namespace {"
> > that the user forgot to close).
> >
> > I'd like to implement both of these ideas as followups, but in
> > the meantime, is the fix-it hint patch OK for trunk?
> > (successfully bootstrapped & regrtested on x86_64-pc-linux-gnu)
>
> Just wondering: how easy would it be to restrict the note to the
> kinds
> of cases you mention? TBH I think clang goes in for extra notes too
> much, and it's not always that case that an "expected 'foo'" message
> really is caused by a missing 'foo'. It'd be great if there was some
> way of making the notes a bit more discerning. :-)
>
> Or maybe do something like restrict the extra note to cases in which
> the
> opening character is on a different line and use an underlined range
> when the opening character is on the same line?
>
> Thanks,
> Richard
Thanks.
This patch implements a new method:
bool gcc_rich_location::add_location_if_nearby (location_t);
to make it easy for a diagnostic to compactly print secondary locations
for these kinds of cases, falling back to printing them via a note
otherwise.
Usage example (adapted from the one in the header):
gcc_rich_location richloc (primary_loc);
bool added secondary = richloc.add_location_if_nearby (secondary_loc);
error_at_rich_loc (&richloc, "missing %qs", "}");
if (!added secondary)
inform (secondary_loc, "here's the associated %qs", "{");
When primary_loc and secondary_loc are on the same line this will print:
test.c:1:39: error: missing '}'
struct same_line { double x; double y; ;
~ ^
When they are on different lines, this will print:
test.c:6:1: error: missing '}'
;
^
test.c:3:1: note: here's the associated '{'
{
^
Successfully bootstrapped®rtested on x86_64-pc-linux-gnu;
takes -fself-test from 39233 passes to 39328 passes.
Committed to trunk as r250133 (and r250134 to fix a ChangeLog snafu).
gcc/ChangeLog:
* diagnostic-show-locus.c: Include "gcc-rich-location.h".
(layout::m_primary_loc): New field.
(layout::layout): Initialize new field. Move location filtering
logic from here to...
(layout::maybe_add_location_range): ...this new method. Add
support for filtering to just the lines already specified by other
locations.
(layout::will_show_line_p): New method.
(gcc_rich_location::add_location_if_nearby): New method.
(selftest::test_add_location_if_nearby): New test function.
(selftest::diagnostic_show_locus_c_tests): Call it.
* gcc-rich-location.h (gcc_rich_location::add_location_if_nearby):
New method.
---
gcc/diagnostic-show-locus.c | 273 +++++++++++++++++++++++++++++++++-----------
gcc/gcc-rich-location.h | 21 ++++
2 files changed, 228 insertions(+), 66 deletions(-)
diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c
index 8a4fd5f..5227400 100644
--- a/gcc/diagnostic-show-locus.c
+++ b/gcc/diagnostic-show-locus.c
@@ -27,6 +27,7 @@ along with GCC; see the file COPYING3. If not see
#include "backtrace.h"
#include "diagnostic.h"
#include "diagnostic-color.h"
+#include "gcc-rich-location.h"
#include "selftest.h"
#ifdef HAVE_TERMIOS_H
@@ -196,6 +197,9 @@ class layout
rich_location *richloc,
diagnostic_t diagnostic_kind);
+ bool maybe_add_location_range (const location_range *loc_range,
+ bool restrict_to_current_line_spans);
+
int get_num_line_spans () const { return m_line_spans.length (); }
const line_span *get_line_span (int idx) const { return &m_line_spans[idx]; }
@@ -206,6 +210,7 @@ class layout
void print_line (int row);
private:
+ bool will_show_line_p (int row) const;
void print_leading_fixits (int row);
void print_source_line (int row, const char *line, int line_width,
line_bounds *lbounds_out);
@@ -241,6 +246,7 @@ class layout
diagnostic_context *m_context;
pretty_printer *m_pp;
diagnostic_t m_diagnostic_kind;
+ location_t m_primary_loc;
expanded_location m_exploc;
colorizer m_colorizer;
bool m_colorize_source_p;
@@ -767,6 +773,7 @@ layout::layout (diagnostic_context * context,
: m_context (context),
m_pp (context->printer),
m_diagnostic_kind (diagnostic_kind),
+ m_primary_loc (richloc->get_range (0)->m_loc),
m_exploc (richloc->get_expanded_location (0)),
m_colorizer (context, diagnostic_kind),
m_colorize_source_p (context->colorize_source_p),
@@ -775,77 +782,12 @@ layout::layout (diagnostic_context * context,
m_line_spans (1 + richloc->get_num_locations ()),
m_x_offset (0)
{
- source_location primary_loc = richloc->get_range (0)->m_loc;
-
for (unsigned int idx = 0; idx < richloc->get_num_locations (); idx++)
{
/* This diagnostic printer can only cope with "sufficiently sane" ranges.
Ignore any ranges that are awkward to handle. */
const location_range *loc_range = richloc->get_range (idx);
-
- /* Split the "range" into caret and range information. */
- source_range src_range = get_range_from_loc (line_table, loc_range->m_loc);
-
- /* Expand the various locations. */
- expanded_location start
- = linemap_client_expand_location_to_spelling_point
- (src_range.m_start, LOCATION_ASPECT_START);
- expanded_location finish
- = linemap_client_expand_location_to_spelling_point
- (src_range.m_finish, LOCATION_ASPECT_FINISH);
- expanded_location caret
- = linemap_client_expand_location_to_spelling_point
- (loc_range->m_loc, LOCATION_ASPECT_CARET);
-
- /* If any part of the range isn't in the same file as the primary
- location of this diagnostic, ignore the range. */
- if (start.file != m_exploc.file)
- continue;
- if (finish.file != m_exploc.file)
- continue;
- if (loc_range->m_show_caret_p)
- if (caret.file != m_exploc.file)
- continue;
-
- /* Sanitize the caret location for non-primary ranges. */
- if (m_layout_ranges.length () > 0)
- if (loc_range->m_show_caret_p)
- if (!compatible_locations_p (loc_range->m_loc, primary_loc))
- /* Discard any non-primary ranges that can't be printed
- sanely relative to the primary location. */
- continue;
-
- /* Everything is now known to be in the correct source file,
- but it may require further sanitization. */
- layout_range ri (&start, &finish, loc_range->m_show_caret_p, &caret);
-
- /* If we have a range that finishes before it starts (perhaps
- from something built via macro expansion), printing the
- range is likely to be nonsensical. Also, attempting to do so
- breaks assumptions within the printing code (PR c/68473).
- Similarly, don't attempt to print ranges if one or both ends
- of the range aren't sane to print relative to the
- primary location (PR c++/70105). */
- if (start.line > finish.line
- || !compatible_locations_p (src_range.m_start, primary_loc)
- || !compatible_locations_p (src_range.m_finish, primary_loc))
- {
- /* Is this the primary location? */
- if (m_layout_ranges.length () == 0)
- {
- /* We want to print the caret for the primary location, but
- we must sanitize away m_start and m_finish. */
- ri.m_start = ri.m_caret;
- ri.m_finish = ri.m_caret;
- }
- else
- /* This is a non-primary range; ignore it. */
- continue;
- }
-
- /* Passed all the tests; add the range to m_layout_ranges so that
- it will be printed. */
- m_layout_ranges.safe_push (ri);
+ maybe_add_location_range (loc_range, false);
}
/* Populate m_fixit_hints, filtering to only those that are in the
@@ -882,6 +824,118 @@ layout::layout (diagnostic_context * context,
show_ruler (m_x_offset + max_width);
}
+/* Attempt to add LOC_RANGE to m_layout_ranges, filtering them to
+ those that we can sanely print.
+
+ If RESTRICT_TO_CURRENT_LINE_SPANS is true, then LOC_RANGE is also
+ filtered against this layout instance's current line spans: it
+ will only be added if the location is fully within the lines
+ already specified by other locations.
+
+ Return true iff LOC_RANGE was added. */
+
+bool
+layout::maybe_add_location_range (const location_range *loc_range,
+ bool restrict_to_current_line_spans)
+{
+ gcc_assert (loc_range);
+
+ /* Split the "range" into caret and range information. */
+ source_range src_range = get_range_from_loc (line_table, loc_range->m_loc);
+
+ /* Expand the various locations. */
+ expanded_location start
+ = linemap_client_expand_location_to_spelling_point
+ (src_range.m_start, LOCATION_ASPECT_START);
+ expanded_location finish
+ = linemap_client_expand_location_to_spelling_point
+ (src_range.m_finish, LOCATION_ASPECT_FINISH);
+ expanded_location caret
+ = linemap_client_expand_location_to_spelling_point
+ (loc_range->m_loc, LOCATION_ASPECT_CARET);
+
+ /* If any part of the range isn't in the same file as the primary
+ location of this diagnostic, ignore the range. */
+ if (start.file != m_exploc.file)
+ return false;
+ if (finish.file != m_exploc.file)
+ return false;
+ if (loc_range->m_show_caret_p)
+ if (caret.file != m_exploc.file)
+ return false;
+
+ /* Sanitize the caret location for non-primary ranges. */
+ if (m_layout_ranges.length () > 0)
+ if (loc_range->m_show_caret_p)
+ if (!compatible_locations_p (loc_range->m_loc, m_primary_loc))
+ /* Discard any non-primary ranges that can't be printed
+ sanely relative to the primary location. */
+ return false;
+
+ /* Everything is now known to be in the correct source file,
+ but it may require further sanitization. */
+ layout_range ri (&start, &finish, loc_range->m_show_caret_p, &caret);
+
+ /* If we have a range that finishes before it starts (perhaps
+ from something built via macro expansion), printing the
+ range is likely to be nonsensical. Also, attempting to do so
+ breaks assumptions within the printing code (PR c/68473).
+ Similarly, don't attempt to print ranges if one or both ends
+ of the range aren't sane to print relative to the
+ primary location (PR c++/70105). */
+ if (start.line > finish.line
+ || !compatible_locations_p (src_range.m_start, m_primary_loc)
+ || !compatible_locations_p (src_range.m_finish, m_primary_loc))
+ {
+ /* Is this the primary location? */
+ if (m_layout_ranges.length () == 0)
+ {
+ /* We want to print the caret for the primary location, but
+ we must sanitize away m_start and m_finish. */
+ ri.m_start = ri.m_caret;
+ ri.m_finish = ri.m_caret;
+ }
+ else
+ /* This is a non-primary range; ignore it. */
+ return false;
+ }
+
+ /* Potentially filter to just the lines already specified by other
+ locations. This is for use by gcc_rich_location::add_location_if_nearby.
+ The layout ctor doesn't use it, and can't because m_line_spans
+ hasn't been set up at that point. */
+ if (restrict_to_current_line_spans)
+ {
+ if (!will_show_line_p (start.line))
+ return false;
+ if (!will_show_line_p (finish.line))
+ return false;
+ if (loc_range->m_show_caret_p)
+ if (!will_show_line_p (caret.line))
+ return false;
+ }
+
+ /* Passed all the tests; add the range to m_layout_ranges so that
+ it will be printed. */
+ m_layout_ranges.safe_push (ri);
+ return true;
+}
+
+/* Return true iff ROW is within one of the line spans for this layout. */
+
+bool
+layout::will_show_line_p (int row) const
+{
+ for (int line_span_idx = 0; line_span_idx < get_num_line_spans ();
+ line_span_idx++)
+ {
+ const line_span *line_span = get_line_span (line_span_idx);
+ if (line_span->contains_line_p (row))
+ return true;
+ }
+ return false;
+}
+
/* Return true iff we should print a heading when starting the
line span with the given index. */
@@ -1782,6 +1836,28 @@ layout::print_line (int row)
} /* End of anonymous namespace. */
+/* If LOC is within the spans of lines that will already be printed for
+ this gcc_rich_location, then add it as a secondary location and return true.
+
+ Otherwise return false. */
+
+bool
+gcc_rich_location::add_location_if_nearby (location_t loc)
+{
+ /* Use the layout location-handling logic to sanitize LOC,
+ filtering it to the current line spans within a temporary
+ layout instance. */
+ layout layout (global_dc, this, DK_ERROR);
+ location_range loc_range;
+ loc_range.m_loc = loc;
+ loc_range.m_show_caret_p = false;
+ if (!layout.maybe_add_location_range (&loc_range, true))
+ return false;
+
+ add_range (loc, false);
+ return true;
+}
+
/* Print the physical source code corresponding to the location of
this diagnostic, with additional annotations. */
@@ -2226,6 +2302,70 @@ test_diagnostic_show_locus_one_liner (const line_table_case &case_)
test_one_liner_many_fixits_2 ();
}
+/* Verify that gcc_rich_location::add_location_if_nearby works. */
+
+static void
+test_add_location_if_nearby (const line_table_case &case_)
+{
+ /* Create a tempfile and write some text to it.
+ ...000000000111111111122222222223333333333.
+ ...123456789012345678901234567890123456789. */
+ const char *content
+ = ("struct same_line { double x; double y; ;\n" /* line 1. */
+ "struct different_line\n" /* line 2. */
+ "{\n" /* line 3. */
+ " double x;\n" /* line 4. */
+ " double y;\n" /* line 5. */
+ ";\n"); /* line 6. */
+ temp_source_file tmp (SELFTEST_LOCATION, ".c", content);
+ line_table_test ltt (case_);
+
+ const line_map_ordinary *ord_map
+ = linemap_check_ordinary (linemap_add (line_table, LC_ENTER, false,
+ tmp.get_filename (), 0));
+
+ linemap_line_start (line_table, 1, 100);
+
+ const location_t final_line_end
+ = linemap_position_for_line_and_column (line_table, ord_map, 6, 7);
+
+ /* Don't attempt to run the tests if column data might be unavailable. */
+ if (final_line_end > LINE_MAP_MAX_LOCATION_WITH_COLS)
+ return;
+
+ /* Test of add_location_if_nearby on the same line as the
+ primary location. */
+ {
+ const location_t missing_close_brace_1_39
+ = linemap_position_for_line_and_column (line_table, ord_map, 1, 39);
+ const location_t matching_open_brace_1_18
+ = linemap_position_for_line_and_column (line_table, ord_map, 1, 18);
+ gcc_rich_location richloc (missing_close_brace_1_39);
+ bool added = richloc.add_location_if_nearby (matching_open_brace_1_18);
+ ASSERT_TRUE (added);
+ ASSERT_EQ (2, richloc.get_num_locations ());
+ test_diagnostic_context dc;
+ diagnostic_show_locus (&dc, &richloc, DK_ERROR);
+ ASSERT_STREQ ("\n"
+ " struct same_line { double x; double y; ;\n"
+ " ~ ^\n",
+ pp_formatted_text (dc.printer));
+ }
+
+ /* Test of add_location_if_nearby on a different line to the
+ primary location. */
+ {
+ const location_t missing_close_brace_6_1
+ = linemap_position_for_line_and_column (line_table, ord_map, 6, 1);
+ const location_t matching_open_brace_3_1
+ = linemap_position_for_line_and_column (line_table, ord_map, 3, 1);
+ gcc_rich_location richloc (missing_close_brace_6_1);
+ bool added = richloc.add_location_if_nearby (matching_open_brace_3_1);
+ ASSERT_FALSE (added);
+ ASSERT_EQ (1, richloc.get_num_locations ());
+ }
+}
+
/* Verify that we print fixits even if they only affect lines
outside those covered by the ranges in the rich_location. */
@@ -2857,6 +2997,7 @@ diagnostic_show_locus_c_tests ()
test_diagnostic_show_locus_unknown_location ();
for_each_line_table_case (test_diagnostic_show_locus_one_liner);
+ for_each_line_table_case (test_add_location_if_nearby);
for_each_line_table_case (test_diagnostic_show_locus_fixit_lines);
for_each_line_table_case (test_fixit_consolidation);
for_each_line_table_case (test_overlapped_fixit_printing);
diff --git a/gcc/gcc-rich-location.h b/gcc/gcc-rich-location.h
index 49708ca..2720f38 100644
--- a/gcc/gcc-rich-location.h
+++ b/gcc/gcc-rich-location.h
@@ -40,6 +40,27 @@ class gcc_rich_location : public rich_location
void add_fixit_misspelled_id (location_t misspelled_token_loc,
tree hint_id);
+
+ /* If LOC is within the spans of lines that will already be printed for
+ this gcc_rich_location, then add it as a secondary location
+ and return true.
+
+ Otherwise return false.
+
+ This allows for a diagnostic to compactly print secondary locations
+ in one diagnostic when these are near enough the primary locations for
+ diagnostics-show-locus.c to cope with them, and to fall back to
+ printing them via a note otherwise e.g.:
+
+ gcc_rich_location richloc (primary_loc);
+ bool added secondary = richloc.add_location_if_nearby (secondary_loc);
+ error_at_rich_loc (&richloc, "main message");
+ if (!added secondary)
+ inform (secondary_loc, "message for secondary");
+
+ Implemented in diagnostic-show-locus.c. */
+
+ bool add_location_if_nearby (location_t loc);
};
#endif /* GCC_RICH_LOCATION_H */
--
1.8.5.3
More information about the Gcc-patches
mailing list