From patchwork Fri Aug 19 21:51:14 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 660990 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3sGGBv701Nz9s9G for ; Sat, 20 Aug 2016 07:22:30 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=LQRvsGlc; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:date:message-id; q=dns; s=default; b=DSWd5+Vm4g1/ xzwFkJS4o98PI+loUQSlIS6a2qv4Ye2Cf3NnFUMRmT8OV6TVPUgXPVzRI2Lo7buj SLbABNd0Qdbm5ijsKkf2ojDK8/6ICJjjYcFxRokriJH7aj8/XcWdz53DlZKHy7dd zI95vMXIOdMZk7vtPLt99wHNwfiYAq0= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:date:message-id; s=default; bh=Zn0Npls2JUntVE9nUG JbnjzYV8s=; b=LQRvsGlcfbti2+8YNHrU0ZLSM4jdC68SSOjSPupxCnjBvO2Vxi W0/5h2VmmymQR7B3pxnIUTHhsHzdokXkeGqNLB7EwFhSaH/myspS+4mfsoiWCVlf q3eM5GMhKNYZ5dpg05PtNqQ1X+005axpafjvoTE8dItoKuWR/k8uHrhjE= Received: (qmail 8934 invoked by alias); 19 Aug 2016 21:22:22 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 8920 invoked by uid 89); 19 Aug 2016 21:22:21 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=removals, equals, pretty_printer, LINE 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; Fri, 19 Aug 2016 21:22:19 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6D99F4E4C2 for ; Fri, 19 Aug 2016 21:22:18 +0000 (UTC) Received: from c64.redhat.com (vpn-229-176.phx2.redhat.com [10.3.229.176]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u7JLMHfZ009577; Fri, 19 Aug 2016 17:22:17 -0400 From: David Malcolm To: gcc-patches@gcc.gnu.org Cc: David Malcolm Subject: [committed] Reimplement removal fix-it hints in terms of replace Date: Fri, 19 Aug 2016 17:51:14 -0400 Message-Id: <1471643474-63177-1-git-send-email-dmalcolm@redhat.com> X-IsSubscribed: yes This patch eliminates class fixit_remove, reimplementing rich_location::add_fixit_remove in terms of replacement with the empty string. Deleting the removal subclass simplifies fixit-handling code, as we only have two concrete fixit_hint subclasses to deal with, rather than three. The patch also fixes some problems in diagnostic-show-locus.c for situations where a replacement fix-it has a different range to the range of the diagnostic, by unifying the drawing of the two kinds of fixits. For example, this: foo = bar.field; ^ m_field becomes: foo = bar.field; ^ ----- m_field showing the range to be replaced. Successfully bootstrapped®rtested on x86_64-pc-linux-gnu. Committed to trunk as r239632. gcc/ChangeLog: * diagnostic-show-locus.c (layout::annotation_line_showed_range_p): New method. (layout::print_any_fixits): Remove case fixit_hint::REMOVE. Reimplement case fixit_hint::REPLACE to cover removals, and replacements where the range of the replacement isn't one of the ranges in the rich_location. (test_one_liner_fixit_replace): Likewise. (selftest::test_one_liner_fixit_replace_non_equal_range): New function. (selftest::test_one_liner_fixit_replace_equal_secondary_range): New function. (selftest::test_diagnostic_show_locus_one_liner): Call the new functions. * diagnostic.c (print_parseable_fixits): Remove case fixit_hint::REMOVE. libcpp/ChangeLog: * include/line-map.h (fixit_hint::kind): Delete REPLACE. (class fixit_remove): Delete. * line-map.c (rich_location::add_fixit_remove): Reimplement by calling add_fixit_replace with an empty string. (fixit_remove::fixit_remove): Delete. (fixit_remove::affects_line_p): Delete. --- gcc/diagnostic-show-locus.c | 124 +++++++++++++++++++++++++++++++++++++------- gcc/diagnostic.c | 4 -- libcpp/include/line-map.h | 23 +------- libcpp/line-map.c | 18 +------ 4 files changed, 106 insertions(+), 63 deletions(-) diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c index 4498f7c..32b1078 100644 --- a/gcc/diagnostic-show-locus.c +++ b/gcc/diagnostic-show-locus.c @@ -199,6 +199,8 @@ class layout bool print_source_line (int row, line_bounds *lbounds_out); void print_annotation_line (int row, const line_bounds lbounds); + bool annotation_line_showed_range_p (int line, int start_column, + int finish_column) const; void print_any_fixits (int row, const rich_location *richloc); void show_ruler (int max_column) const; @@ -1053,6 +1055,26 @@ layout::print_annotation_line (int row, const line_bounds lbounds) print_newline (); } +/* Subroutine of layout::print_any_fixits. + + Determine if the annotation line printed for LINE contained + the exact range from START_COLUMN to FINISH_COLUMN. */ + +bool +layout::annotation_line_showed_range_p (int line, int start_column, + int finish_column) const +{ + layout_range *range; + int i; + FOR_EACH_VEC_ELT (m_layout_ranges, i, range) + if (range->m_start.m_line == line + && range->m_start.m_column == start_column + && range->m_finish.m_line == line + && range->m_finish.m_column == finish_column) + return true; + return false; +} + /* If there are any fixit hints on source line ROW within RICHLOC, print them. They are printed in order, attempting to combine them onto lines, but starting new lines if necessary. */ @@ -1083,33 +1105,39 @@ layout::print_any_fixits (int row, const rich_location *richloc) } break; - case fixit_hint::REMOVE: + case fixit_hint::REPLACE: { - fixit_remove *remove = static_cast (hint); - /* This assumes the removal just affects one line. */ - source_range src_range = remove->get_range (); + fixit_replace *replace = static_cast (hint); + source_range src_range = replace->get_range (); + int line = LOCATION_LINE (src_range.m_start); int start_column = LOCATION_COLUMN (src_range.m_start); int finish_column = LOCATION_COLUMN (src_range.m_finish); - move_to_column (&column, start_column); - for (int column = start_column; column <= finish_column; column++) + + /* If the range of the replacement wasn't printed in the + annotation line, then print an extra underline to + indicate exactly what is being replaced. + Always show it for removals. */ + if (!annotation_line_showed_range_p (line, start_column, + finish_column) + || replace->get_length () == 0) { + move_to_column (&column, start_column); m_colorizer.set_fixit_hint (); - pp_character (m_pp, '-'); + for (; column <= finish_column; column++) + pp_character (m_pp, '-'); m_colorizer.set_normal_text (); } - } - break; - - case fixit_hint::REPLACE: - { - fixit_replace *replace = static_cast (hint); - int start_column - = LOCATION_COLUMN (replace->get_range ().m_start); - move_to_column (&column, start_column); - m_colorizer.set_fixit_hint (); - pp_string (m_pp, replace->get_string ()); - m_colorizer.set_normal_text (); - column += replace->get_length (); + /* Print the replacement text. REPLACE also covers + removals, so only do this extra work (potentially starting + a new line) if we have actual replacement text. */ + if (replace->get_length () > 0) + { + move_to_column (&column, start_column); + m_colorizer.set_fixit_hint (); + pp_string (m_pp, replace->get_string ()); + m_colorizer.set_normal_text (); + column += replace->get_length (); + } } break; @@ -1502,6 +1530,60 @@ test_one_liner_fixit_replace () pp_formatted_text (dc.printer)); } +/* Replace fix-it hint: replacing "field" with "m_field", + but where the caret was elsewhere. */ + +static void +test_one_liner_fixit_replace_non_equal_range () +{ + test_diagnostic_context dc; + location_t equals = linemap_position_for_column (line_table, 5); + location_t start = linemap_position_for_column (line_table, 11); + location_t finish = linemap_position_for_column (line_table, 15); + rich_location richloc (line_table, equals); + source_range range; + range.m_start = start; + range.m_finish = finish; + richloc.add_fixit_replace (range, "m_field"); + diagnostic_show_locus (&dc, &richloc, DK_ERROR); + /* The replacement range is not indicated in the annotation line, so + it should be indicated via an additional underline. */ + ASSERT_STREQ ("\n" + " foo = bar.field;\n" + " ^\n" + " -----\n" + " m_field\n", + pp_formatted_text (dc.printer)); +} + +/* Replace fix-it hint: replacing "field" with "m_field", + where the caret was elsewhere, but where a secondary range + exactly covers "field". */ + +static void +test_one_liner_fixit_replace_equal_secondary_range () +{ + test_diagnostic_context dc; + location_t equals = linemap_position_for_column (line_table, 5); + location_t start = linemap_position_for_column (line_table, 11); + location_t finish = linemap_position_for_column (line_table, 15); + rich_location richloc (line_table, equals); + location_t field = make_location (start, start, finish); + richloc.add_range (field, false); + source_range range; + range.m_start = start; + range.m_finish = finish; + richloc.add_fixit_replace (range, "m_field"); + diagnostic_show_locus (&dc, &richloc, DK_ERROR); + /* The replacement range is indicated in the annotation line, + so it shouldn't be indicated via an additional underline. */ + ASSERT_STREQ ("\n" + " foo = bar.field;\n" + " ^ ~~~~~\n" + " m_field\n", + pp_formatted_text (dc.printer)); +} + /* Run the various one-liner tests. */ static void @@ -1532,6 +1614,8 @@ test_diagnostic_show_locus_one_liner (const line_table_case &case_) test_one_liner_fixit_insert (); test_one_liner_fixit_remove (); test_one_liner_fixit_replace (); + test_one_liner_fixit_replace_non_equal_range (); + test_one_liner_fixit_replace_equal_secondary_range (); } /* Run all of the selftests within this file. */ diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c index fec48c4..b47da38 100644 --- a/gcc/diagnostic.c +++ b/gcc/diagnostic.c @@ -758,10 +758,6 @@ print_parseable_fixits (pretty_printer *pp, rich_location *richloc) } break; - case fixit_hint::REMOVE: - print_escaped_string (pp, ""); - break; - case fixit_hint::REPLACE: { const fixit_replace *replace diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h index 443086a..f65931c 100644 --- a/libcpp/include/line-map.h +++ b/libcpp/include/line-map.h @@ -1422,7 +1422,7 @@ protected: class fixit_hint { public: - enum kind {INSERT, REMOVE, REPLACE}; + enum kind {INSERT, REPLACE}; virtual ~fixit_hint () {} @@ -1453,27 +1453,6 @@ class fixit_insert : public fixit_hint size_t m_len; }; -class fixit_remove : public fixit_hint -{ - public: - fixit_remove (source_range src_range); - ~fixit_remove () {} - - enum kind get_kind () const { return REMOVE; } - bool affects_line_p (const char *file, int line); - source_location get_start_loc () const { return m_src_range.m_start; } - bool maybe_get_end_loc (source_location *out) const - { - *out = m_src_range.m_finish; - return true; - } - - source_range get_range () const { return m_src_range; } - - private: - source_range m_src_range; -}; - class fixit_replace : public fixit_hint { public: diff --git a/libcpp/line-map.c b/libcpp/line-map.c index 141af9d..3890eff 100644 --- a/libcpp/line-map.c +++ b/libcpp/line-map.c @@ -2086,8 +2086,7 @@ rich_location::add_fixit_insert (source_location where, void rich_location::add_fixit_remove (source_range src_range) { - linemap_assert (m_num_fixit_hints < MAX_FIXIT_HINTS); - m_fixit_hints[m_num_fixit_hints++] = new fixit_remove (src_range); + add_fixit_replace (src_range, ""); } /* Add a fixit-hint, suggesting replacement of the content at @@ -2130,21 +2129,6 @@ fixit_insert::affects_line_p (const char *file, int line) return false; } -/* class fixit_remove. */ - -fixit_remove::fixit_remove (source_range src_range) -: m_src_range (src_range) -{ -} - -/* Implementation of fixit_hint::affects_line_p for fixit_remove. */ - -bool -fixit_remove::affects_line_p (const char *file, int line) -{ - return m_src_range.intersects_line_p (file, line); -} - /* class fixit_replace. */ fixit_replace::fixit_replace (source_range src_range,