From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31490 invoked by alias); 11 Jul 2017 14:09:29 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 30839 invoked by uid 89); 11 Jul 2017 14:09:29 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=sk:layout, 7737, U*dmalcolm, sk:dmalcol X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 11 Jul 2017 14:09:26 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B29938047E; Tue, 11 Jul 2017 14:09:24 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com B29938047E Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=dmalcolm@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com B29938047E Received: from c64.redhat.com (ovpn-112-25.phx2.redhat.com [10.3.112.25]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7A58118373; Tue, 11 Jul 2017 14:09:23 +0000 (UTC) From: David Malcolm To: Richard Sandiford Cc: gcc-patches@gcc.gnu.org, David Malcolm Subject: [committed] diagnostics: support compact printing of secondary locations Date: Tue, 11 Jul 2017 14:09:00 -0000 Message-Id: <1499784198-36796-1-git-send-email-dmalcolm@redhat.com> In-Reply-To: <87y3s5pcvm.fsf@linaro.org> References: <87y3s5pcvm.fsf@linaro.org> X-IsSubscribed: yes X-SW-Source: 2017-07/txt/msg00525.txt.bz2 On Mon, 2017-07-03 at 19:57 +0100, Richard Sandiford wrote: > [Thanks for all your diagnostic work btw.] > > David Malcolm 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