From ad53f12355a61ad5811d59977c963a16b6c5be8d Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Tue, 2 May 2017 19:03:56 +0000 Subject: [PATCH] Support fix-it hints that add new lines Previously fix-it hints couldn't contain newlines. This is due to the need to print something user-readable for them within diagnostic-show-locus, and for handling them within edit-context for printing diffs and regenerating content. This patch enables limited support for fix-it hints with newlines, for suggesting adding new lines. Such a fix-it hint must have exactly one newline character, at the end of the content. It must be an insertion at the beginning of a line (so that e.g. fix-its that split a pre-existing line are still rejected). They are printed by diagnostic-show-locus with a '+' in the left-hand margin, like this: test.c:42:4: note: suggest adding 'break;' here + break; case 'b': ^~~~~~~~~ and the printer injects "spans" if the insertion location is not near the primary range of the diagnostic e.g.: test.c:4:2: note: unrecognized 'putchar'; suggest including '' test.c:1:1: +#include test.c:4:2: putchar (ch); ^~~~~~~ gcc/ChangeLog: * diagnostic-show-locus.c (layout::should_print_annotation_line_p): Make private. (layout::print_annotation_line): Make private. (layout::annotation_line_showed_range_p): Make private. (layout::show_ruler): Make private. (layout::print_source_line): Make private. Pass in line and line_width, rather than calling location_get_source_line. Drop returned value. (layout::print_leading_fixits): New method. (layout::print_any_fixits): Rename to... (layout::print_trailing_fixits): ...this, and make private. Don't print newline fixits. (diagnostic_show_locus): Move logic for printing one row into... (layout::print_line): ...this new function. Move the location_get_source_line call and error-handling from print_source_line to here. Call print_leading_fixits, and rename print_any_fixits to print_trailing_fixits. (selftest::test_fixit_insert_containing_newline): Update now that newlines are partially supported. (selftest::test_fixit_insert_containing_newline_2): New test. (selftest::test_fixit_replace_containing_newline): Update comments. (selftest::diagnostic_show_locus_c_tests): Call the new test. * edit-context.c (class added_line): New class. (class edited_line): Describe newline handling in comment. (edited_line::actually_edited_p): New method. (edited_line::print_content): Delete redundant decl. (edited_line::m_predecessors): New field. (edited_file::print_content): Call edited_line::print_content. (edited_file::print_diff): Update to support newlines. (edited_file::print_diff_hunk): Likewise. (edited_file::print_run_of_changed_lines): New function. (edited_file::print_diff_line): Convert to... (print_diff_line): ...this. (edited_file::get_effective_line_count): New function. (edited_line::edited_line): Initialize new field m_predecessors. (edited_line::~edited_line): Clean up m_predecessors. (edited_line::apply_fixit): Handle newlines. (edited_line::get_effective_line_count): New function. (edited_line::print_content): New function. (edited_line::print_diff_lines): New function. (selftest::test_applying_fixits_insert_containing_newline): New test. (selftest::test_applying_fixits_replace_containing_newline): New test. (selftest::insert_line): New function. (selftest::test_applying_fixits_multiple_lines): Add example of inserting a line. (selftest::edit_context_c_tests): Call the new tests. gcc/testsuite/ChangeLog: * gcc.dg/plugin/diagnostic-test-show-locus-bw.c (test_fixit_insert_newline): New function. * gcc.dg/plugin/diagnostic-test-show-locus-color.c (test_fixit_insert_newline): New function. * gcc.dg/plugin/diagnostic-test-show-locus-generate-patch.c (test_fixit_insert_newline): New function. * gcc.dg/plugin/diagnostic-test-show-locus-parseable-fixits.c (test_fixit_insert_newline): New function. * gcc.dg/plugin/diagnostic_plugin_test_show_locus.c (test_show_locus): Handle test_fixit_insert_newline. libcpp/ChangeLog: * include/line-map.h (class rich_location): Update description of newline handling. (class fixit_hint): Likewise. (fixit_hint::ends_with_newline_p): New decl. * line-map.c (rich_location::maybe_add_fixit): Support newlines in fix-it hints that are insertions of single lines at the start of a line. Don't consolidate into such fix-it hints. (fixit_hint::ends_with_newline_p): New method. From-SVN: r247522 --- gcc/ChangeLog | 51 +++ gcc/diagnostic-show-locus.c | 211 +++++++--- gcc/edit-context.c | 394 +++++++++++++++--- gcc/testsuite/ChangeLog | 13 + .../plugin/diagnostic-test-show-locus-bw.c | 20 + .../plugin/diagnostic-test-show-locus-color.c | 20 + ...iagnostic-test-show-locus-generate-patch.c | 21 + ...gnostic-test-show-locus-parseable-fixits.c | 16 + .../diagnostic_plugin_test_show_locus.c | 12 + libcpp/ChangeLog | 11 + libcpp/include/line-map.h | 16 +- libcpp/line-map.c | 50 ++- 12 files changed, 727 insertions(+), 108 deletions(-) diff --git a/gcc/ChangeLog b/gcc/ChangeLog index f4a64804589..2b6dfb3a45c 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,54 @@ +2017-05-02 David Malcolm + + * diagnostic-show-locus.c + (layout::should_print_annotation_line_p): Make private. + (layout::print_annotation_line): Make private. + (layout::annotation_line_showed_range_p): Make private. + (layout::show_ruler): Make private. + (layout::print_source_line): Make private. Pass in line and + line_width, rather than calling location_get_source_line. Drop + returned value. + (layout::print_leading_fixits): New method. + (layout::print_any_fixits): Rename to... + (layout::print_trailing_fixits): ...this, and make private. + Don't print newline fixits. + (diagnostic_show_locus): Move logic for printing one row into... + (layout::print_line): ...this new function. Move the + location_get_source_line call and error-handling from + print_source_line to here. Call print_leading_fixits, and rename + print_any_fixits to print_trailing_fixits. + (selftest::test_fixit_insert_containing_newline): Update now that + newlines are partially supported. + (selftest::test_fixit_insert_containing_newline_2): New test. + (selftest::test_fixit_replace_containing_newline): Update comments. + (selftest::diagnostic_show_locus_c_tests): Call the new test. + * edit-context.c (class added_line): New class. + (class edited_line): Describe newline handling in comment. + (edited_line::actually_edited_p): New method. + (edited_line::print_content): Delete redundant decl. + (edited_line::m_predecessors): New field. + (edited_file::print_content): Call edited_line::print_content. + (edited_file::print_diff): Update to support newlines. + (edited_file::print_diff_hunk): Likewise. + (edited_file::print_run_of_changed_lines): New function. + (edited_file::print_diff_line): Convert to... + (print_diff_line): ...this. + (edited_file::get_effective_line_count): New function. + (edited_line::edited_line): Initialize new field m_predecessors. + (edited_line::~edited_line): Clean up m_predecessors. + (edited_line::apply_fixit): Handle newlines. + (edited_line::get_effective_line_count): New function. + (edited_line::print_content): New function. + (edited_line::print_diff_lines): New function. + (selftest::test_applying_fixits_insert_containing_newline): New + test. + (selftest::test_applying_fixits_replace_containing_newline): New + test. + (selftest::insert_line): New function. + (selftest::test_applying_fixits_multiple_lines): Add example of + inserting a line. + (selftest::edit_context_c_tests): Call the new tests. + 2017-05-02 Bin Cheng * tree-ssa-loop-ivopts.c (get_scaled_computation_cost_at): Delete diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c index 3c10b69c232..b91c9d55da3 100644 --- a/gcc/diagnostic-show-locus.c +++ b/gcc/diagnostic-show-locus.c @@ -203,16 +203,20 @@ class layout expanded_location get_expanded_location (const line_span *) const; - bool print_source_line (int row, line_bounds *lbounds_out); + void print_line (int row); + + private: + void print_leading_fixits (int row); + void print_source_line (int row, const char *line, int line_width, + line_bounds *lbounds_out); bool should_print_annotation_line_p (int row) const; void print_annotation_line (int row, const line_bounds lbounds); + void print_trailing_fixits (int row); + bool annotation_line_showed_range_p (int line, int start_column, int finish_column) const; - void print_any_fixits (int row); - void show_ruler (int max_column) const; - private: bool validate_fixit_hint_p (const fixit_hint *hint); void calculate_line_spans (); @@ -1055,21 +1059,15 @@ layout::calculate_line_spans () } } -/* Attempt to print line ROW of source code, potentially colorized at any - ranges. - Return true if the line was printed, populating *LBOUNDS_OUT. - Return false if the source line could not be read, leaving *LBOUNDS_OUT - untouched. */ +/* Print line ROW of source code, potentially colorized at any ranges, and + populate *LBOUNDS_OUT. + LINE is the source line (not necessarily 0-terminated) and LINE_WIDTH + is its width. */ -bool -layout::print_source_line (int row, line_bounds *lbounds_out) +void +layout::print_source_line (int row, const char *line, int line_width, + line_bounds *lbounds_out) { - int line_width; - const char *line = location_get_source_line (m_exploc.file, row, - &line_width); - if (!line) - return false; - m_colorizer.set_normal_text (); /* We will stop printing the source line at any trailing @@ -1124,7 +1122,6 @@ layout::print_source_line (int row, line_bounds *lbounds_out) lbounds_out->m_first_non_ws = first_non_ws; lbounds_out->m_last_non_ws = last_non_ws; - return true; } /* Determine if we should print an annotation line for ROW. @@ -1186,7 +1183,46 @@ layout::print_annotation_line (int row, const line_bounds lbounds) print_newline (); } -/* Subroutine of layout::print_any_fixits. +/* If there are any fixit hints inserting new lines before source line ROW, + print them. + + They are printed on lines of their own, before the source line + itself, with a leading '+'. */ + +void +layout::print_leading_fixits (int row) +{ + for (unsigned int i = 0; i < m_fixit_hints.length (); i++) + { + const fixit_hint *hint = m_fixit_hints[i]; + + if (!hint->ends_with_newline_p ()) + /* Not a newline fixit; print it in print_trailing_fixits. */ + continue; + + gcc_assert (hint->insertion_p ()); + + if (hint->affects_line_p (m_exploc.file, row)) + { + /* Printing the '+' with normal colorization + and the inserted line with "insert" colorization + helps them stand out from each other, and from + the surrounding text. */ + m_colorizer.set_normal_text (); + pp_character (m_pp, '+'); + m_colorizer.set_fixit_insert (); + /* Print all but the trailing newline of the fix-it hint. + We have to print the newline separately to avoid + getting additional pp prefixes printed. */ + for (size_t i = 0; i < hint->get_length () - 1; i++) + pp_character (m_pp, hint->get_string ()[i]); + m_colorizer.set_normal_text (); + pp_newline (m_pp); + } + } +} + +/* Subroutine of layout::print_trailing_fixits. Determine if the annotation line printed for LINE contained the exact range from START_COLUMN to FINISH_COLUMN. */ @@ -1208,15 +1244,21 @@ layout::annotation_line_showed_range_p (int line, int start_column, /* If there are any fixit hints on source line ROW, print them. They are printed in order, attempting to combine them onto lines, but - starting new lines if necessary. */ + starting new lines if necessary. + Fix-it hints that insert new lines are handled separately, + in layout::print_leading_fixits. */ void -layout::print_any_fixits (int row) +layout::print_trailing_fixits (int row) { int column = 0; for (unsigned int i = 0; i < m_fixit_hints.length (); i++) { const fixit_hint *hint = m_fixit_hints[i]; + + if (hint->ends_with_newline_p ()) + continue; + if (hint->affects_line_p (m_exploc.file, row)) { /* For now we assume each fixit hint can only touch one line. */ @@ -1416,6 +1458,27 @@ layout::show_ruler (int max_column) const pp_newline (m_pp); } +/* Print leading fix-its (for new lines inserted before the source line) + then the source line, followed by an annotation line + consisting of any caret/underlines, then any fixits. + If the source line can't be read, print nothing. */ +void +layout::print_line (int row) +{ + int line_width; + const char *line = location_get_source_line (m_exploc.file, row, + &line_width); + if (!line) + return; + + line_bounds lbounds; + print_leading_fixits (row); + print_source_line (row, line, line_width, &lbounds); + if (should_print_annotation_line_p (row)) + print_annotation_line (row, lbounds); + print_trailing_fixits (row); +} + } /* End of anonymous namespace. */ /* Print the physical source code corresponding to the location of @@ -1460,18 +1523,7 @@ diagnostic_show_locus (diagnostic_context * context, } int last_line = line_span->get_last_line (); for (int row = line_span->get_first_line (); row <= last_line; row++) - { - /* Print the source line, followed by an annotation line - consisting of any caret/underlines, then any fixits. - If the source line can't be read, print nothing. */ - line_bounds lbounds; - if (layout.print_source_line (row, &lbounds)) - { - if (layout.should_print_annotation_line_p (row)) - layout.print_annotation_line (row, lbounds); - layout.print_any_fixits (row); - } - } + layout.print_line (row); } pp_set_prefix (context->printer, saved_prefix); @@ -2101,8 +2153,7 @@ test_fixit_consolidation (const line_table_case &case_) } } -/* Insertion fix-it hint: adding a "break;" on a line by itself. - This will fail, as newlines aren't yet supported. */ +/* Insertion fix-it hint: adding a "break;" on a line by itself. */ static void test_fixit_insert_containing_newline (const line_table_case &case_) @@ -2119,32 +2170,97 @@ test_fixit_insert_containing_newline (const line_table_case &case_) line_table_test ltt (case_); linemap_add (line_table, LC_ENTER, false, tmp.get_filename (), 3); - /* Add a "break;" on a line by itself before line 3 i.e. before - column 1 of line 3. */ location_t case_start = linemap_position_for_column (line_table, 5); location_t case_finish = linemap_position_for_column (line_table, 13); location_t case_loc = make_location (case_start, case_start, case_finish); - rich_location richloc (line_table, case_loc); location_t line_start = linemap_position_for_column (line_table, 1); - richloc.add_fixit_insert_before (line_start, " break;\n"); - - /* Newlines are not yet supported within fix-it hints, so - the fix-it should not be displayed. */ - ASSERT_TRUE (richloc.seen_impossible_fixit_p ()); if (case_finish > LINE_MAP_MAX_LOCATION_WITH_COLS) return; + /* Add a "break;" on a line by itself before line 3 i.e. before + column 1 of line 3. */ + { + rich_location richloc (line_table, case_loc); + richloc.add_fixit_insert_before (line_start, " break;\n"); + test_diagnostic_context dc; + diagnostic_show_locus (&dc, &richloc, DK_ERROR); + ASSERT_STREQ ("\n" + "+ break;\n" + " case 'b':\n" + " ^~~~~~~~~\n", + pp_formatted_text (dc.printer)); + } + + /* Verify that attempts to add text with a newline fail when the + insertion point is *not* at the start of a line. */ + { + rich_location richloc (line_table, case_loc); + richloc.add_fixit_insert_before (case_start, "break;\n"); + ASSERT_TRUE (richloc.seen_impossible_fixit_p ()); + test_diagnostic_context dc; + diagnostic_show_locus (&dc, &richloc, DK_ERROR); + ASSERT_STREQ ("\n" + " case 'b':\n" + " ^~~~~~~~~\n", + pp_formatted_text (dc.printer)); + } +} + +/* Insertion fix-it hint: adding a "#include \n" to the top + of the file, where the fix-it is printed in a different line-span + to the primary range of the diagnostic. */ + +static void +test_fixit_insert_containing_newline_2 (const line_table_case &case_) +{ + /* Create a tempfile and write some text to it. + .........................0000000001111111. + .........................1234567890123456. */ + const char *old_content = ("test (int ch)\n" /* line 1. */ + "{\n" /* line 2. */ + " putchar (ch);\n" /* line 3. */ + "}\n"); /* line 4. */ + + temp_source_file tmp (SELFTEST_LOCATION, ".c", old_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); + + /* The primary range is the "putchar" token. */ + location_t putchar_start + = linemap_position_for_line_and_column (line_table, ord_map, 3, 2); + location_t putchar_finish + = linemap_position_for_line_and_column (line_table, ord_map, 3, 8); + location_t putchar_loc + = make_location (putchar_start, putchar_start, putchar_finish); + rich_location richloc (line_table, putchar_loc); + + /* Add a "#include " on a line by itself at the top of the file. */ + location_t file_start + = linemap_position_for_line_and_column (line_table, ord_map, 1, 1); + richloc.add_fixit_insert_before (file_start, "#include \n"); + + if (putchar_finish > LINE_MAP_MAX_LOCATION_WITH_COLS) + return; + test_diagnostic_context dc; diagnostic_show_locus (&dc, &richloc, DK_ERROR); ASSERT_STREQ ("\n" - " case 'b':\n" - " ^~~~~~~~~\n", + "FILENAME:1:1:\n" + "+#include \n" + " test (int ch)\n" + "FILENAME:3:2:\n" + " putchar (ch);\n" + " ^~~~~~~\n", pp_formatted_text (dc.printer)); } /* Replacement fix-it hint containing a newline. - This will fail, as newlines aren't yet supported. */ + This will fail, as newlines are only supported when inserting at the + beginning of a line. */ static void test_fixit_replace_containing_newline (const line_table_case &case_) @@ -2167,7 +2283,7 @@ test_fixit_replace_containing_newline (const line_table_case &case_) source_range range = source_range::from_locations (start, finish); richloc.add_fixit_replace (range, "\n ="); - /* Newlines are not yet supported within fix-it hints, so + /* Arbitrary newlines are not yet supported within fix-it hints, so the fix-it should not be displayed. */ ASSERT_TRUE (richloc.seen_impossible_fixit_p ()); @@ -2199,6 +2315,7 @@ diagnostic_show_locus_c_tests () 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_fixit_insert_containing_newline); + for_each_line_table_case (test_fixit_insert_containing_newline_2); for_each_line_table_case (test_fixit_replace_containing_newline); } diff --git a/gcc/edit-context.c b/gcc/edit-context.c index bea8a8af688..6f35ca291f1 100644 --- a/gcc/edit-context.c +++ b/gcc/edit-context.c @@ -86,23 +86,52 @@ class edited_file private: bool print_content (pretty_printer *pp); void print_diff (pretty_printer *pp, bool show_filenames); - void print_diff_hunk (pretty_printer *pp, int start_of_hunk, - int end_of_hunk); - void print_diff_line (pretty_printer *pp, char prefix_char, - const char *line, int line_size); + int print_diff_hunk (pretty_printer *pp, int old_start_of_hunk, + int old_end_of_hunk, int new_start_of_hunk); edited_line *get_line (int line); edited_line *get_or_insert_line (int line); int get_num_lines (bool *missing_trailing_newline); + int get_effective_line_count (int old_start_of_hunk, + int old_end_of_hunk); + + void print_run_of_changed_lines (pretty_printer *pp, + int start_of_run, + int end_of_run); + const char *m_filename; typed_splay_tree m_edited_lines; int m_num_lines; }; +/* A line added before an edited_line. */ + +class added_line +{ + public: + added_line (const char *content, int len) + : m_content (xstrndup (content, len)), m_len (len) {} + ~added_line () { free (m_content); } + + const char *get_content () const { return m_content; } + int get_len () const { return m_len; } + + private: + char *m_content; + int m_len; +}; + /* The state of one edited line within an edited_file. As well as the current content of the line, it contains a record of the changes, so that further changes can be applied in the correct - place. */ + place. + + When handling fix-it hints containing newlines, new lines are added + as added_line predecessors to an edited_line. Hence it's possible + for an "edited_line" to not actually have been changed, but to merely + be a placeholder for the lines added before it. This can be tested + for with actuall_edited_p, and has a slight effect on how diff hunks + are generated. */ class edited_line { @@ -121,16 +150,25 @@ class edited_line const char *replacement_str, int replacement_len); + int get_effective_line_count () const; + + /* Has the content of this line actually changed, or are we merely + recording predecessor added_lines? */ + bool actually_edited_p () const { return m_line_events.length () > 0; } + + void print_content (pretty_printer *pp) const; + void print_diff_lines (pretty_printer *pp) const; + private: void ensure_capacity (int len); void ensure_terminated (); - void print_content (pretty_printer *pp) const; int m_line_num; char *m_content; int m_len; int m_alloc_sz; auto_vec m_line_events; + auto_vec m_predecessors; }; /* Class for representing edit events that have occurred on one line of @@ -160,6 +198,12 @@ class line_event int m_delta; }; +/* Forward decls. */ + +static void +print_diff_line (pretty_printer *pp, char prefix_char, + const char *line, int line_size); + /* Implementation of class edit_context. */ /* edit_context's ctor. */ @@ -375,7 +419,7 @@ edited_file::print_content (pretty_printer *pp) { edited_line *el = get_line (line_num); if (el) - pp_string (pp, el->get_content ()); + el->print_content (pp); else { int len; @@ -417,6 +461,10 @@ edited_file::print_diff (pretty_printer *pp, bool show_filenames) const int context_lines = 3; + /* Track new line numbers minus old line numbers. */ + + int line_delta = 0; + while (el) { int start_of_hunk = el->get_line_num (); @@ -432,39 +480,53 @@ edited_file::print_diff (pretty_printer *pp, bool show_filenames) = m_edited_lines.successor (el->get_line_num ()); if (!next_el) break; - if (el->get_line_num () + context_lines + + int end_of_printed_hunk = el->get_line_num () + context_lines; + if (!el->actually_edited_p ()) + end_of_printed_hunk--; + + if (end_of_printed_hunk >= next_el->get_line_num () - context_lines) el = next_el; else break; } + int end_of_hunk = el->get_line_num (); end_of_hunk += context_lines; + if (!el->actually_edited_p ()) + end_of_hunk--; if (end_of_hunk > line_count) end_of_hunk = line_count; - print_diff_hunk (pp, start_of_hunk, end_of_hunk); - + int new_start_of_hunk = start_of_hunk + line_delta; + line_delta += print_diff_hunk (pp, start_of_hunk, end_of_hunk, + new_start_of_hunk); el = m_edited_lines.successor (el->get_line_num ()); } } /* Print one hunk within a unified diff to PP, covering the - given range of lines. */ + given range of lines. OLD_START_OF_HUNK and OLD_END_OF_HUNK are + line numbers in the unedited version of the file. + NEW_START_OF_HUNK is a line number in the edited version of the file. + Return the change in the line count within the hunk. */ -void -edited_file::print_diff_hunk (pretty_printer *pp, int start_of_hunk, - int end_of_hunk) +int +edited_file::print_diff_hunk (pretty_printer *pp, int old_start_of_hunk, + int old_end_of_hunk, int new_start_of_hunk) { - int num_lines = end_of_hunk - start_of_hunk + 1; + int old_num_lines = old_end_of_hunk - old_start_of_hunk + 1; + int new_num_lines + = get_effective_line_count (old_start_of_hunk, old_end_of_hunk); pp_string (pp, colorize_start (pp_show_color (pp), "diff-hunk")); - pp_printf (pp, "@@ -%i,%i +%i,%i @@\n", start_of_hunk, num_lines, - start_of_hunk, num_lines); + pp_printf (pp, "@@ -%i,%i +%i,%i @@\n", old_start_of_hunk, old_num_lines, + new_start_of_hunk, new_num_lines); pp_string (pp, colorize_stop (pp_show_color (pp))); - int line_num = start_of_hunk; - while (line_num <= end_of_hunk) + int line_num = old_start_of_hunk; + while (line_num <= old_end_of_hunk) { edited_line *el = get_line (line_num); if (el) @@ -475,34 +537,8 @@ edited_file::print_diff_hunk (pretty_printer *pp, int start_of_hunk, while (get_line (line_num)) line_num++; const int last_changed_line_in_run = line_num - 1; - - /* Show old version of lines. */ - pp_string (pp, colorize_start (pp_show_color (pp), - "diff-delete")); - for (line_num = first_changed_line_in_run; - line_num <= last_changed_line_in_run; - line_num++) - { - int line_len; - const char *old_line - = location_get_source_line (m_filename, line_num, &line_len); - print_diff_line (pp, '-', old_line, line_len); - } - pp_string (pp, colorize_stop (pp_show_color (pp))); - - /* Show new version of lines. */ - pp_string (pp, colorize_start (pp_show_color (pp), - "diff-insert")); - for (line_num = first_changed_line_in_run; - line_num <= last_changed_line_in_run; - line_num++) - { - edited_line *el_in_run = get_line (line_num); - gcc_assert (el_in_run); - print_diff_line (pp, '+', el_in_run->get_content (), - el_in_run->get_len ()); - } - pp_string (pp, colorize_stop (pp_show_color (pp))); + print_run_of_changed_lines (pp, first_changed_line_in_run, + last_changed_line_in_run); } else { @@ -514,15 +550,59 @@ edited_file::print_diff_hunk (pretty_printer *pp, int start_of_hunk, line_num++; } } + + return new_num_lines - old_num_lines; +} + +/* Subroutine of edited_file::print_diff_hunk: given a run of lines + from START_OF_RUN to END_OF_RUN that all have edited_line instances, + print the diff to PP. */ + +void +edited_file::print_run_of_changed_lines (pretty_printer *pp, + int start_of_run, + int end_of_run) +{ + /* Show old version of lines. */ + pp_string (pp, colorize_start (pp_show_color (pp), + "diff-delete")); + for (int line_num = start_of_run; + line_num <= end_of_run; + line_num++) + { + edited_line *el_in_run = get_line (line_num); + gcc_assert (el_in_run); + if (el_in_run->actually_edited_p ()) + { + int line_len; + const char *old_line + = location_get_source_line (m_filename, line_num, &line_len); + print_diff_line (pp, '-', old_line, line_len); + } + } + pp_string (pp, colorize_stop (pp_show_color (pp))); + + /* Show new version of lines. */ + pp_string (pp, colorize_start (pp_show_color (pp), + "diff-insert")); + for (int line_num = start_of_run; + line_num <= end_of_run; + line_num++) + { + edited_line *el_in_run = get_line (line_num); + gcc_assert (el_in_run); + el_in_run->print_diff_lines (pp); + } + pp_string (pp, colorize_stop (pp_show_color (pp))); } /* Print one line within a diff, starting with PREFIX_CHAR, followed by the LINE of content, of length LEN. LINE is not necessarily 0-terminated. Print a trailing newline. */ -void -edited_file::print_diff_line (pretty_printer *pp, char prefix_char, - const char *line, int len) +static void +print_diff_line (pretty_printer *pp, char prefix_char, + const char *line, int len) { pp_character (pp, prefix_char); for (int i = 0; i < len; i++) @@ -530,6 +610,27 @@ edited_file::print_diff_line (pretty_printer *pp, char prefix_char, pp_character (pp, '\n'); } +/* Determine the number of lines that will be present after + editing for the range of lines from OLD_START_OF_HUNK to + OLD_END_OF_HUNK inclusive. */ + +int +edited_file::get_effective_line_count (int old_start_of_hunk, + int old_end_of_hunk) +{ + int line_count = 0; + for (int old_line_num = old_start_of_hunk; old_line_num <= old_end_of_hunk; + old_line_num++) + { + edited_line *el = get_line (old_line_num); + if (el) + line_count += el->get_effective_line_count (); + else + line_count++; + } + return line_count; +} + /* Get the state of LINE within the file, or NULL if it is untouched. */ edited_line * @@ -591,7 +692,8 @@ edited_file::get_num_lines (bool *missing_trailing_newline) edited_line::edited_line (const char *filename, int line_num) : m_line_num (line_num), m_content (NULL), m_len (0), m_alloc_sz (0), - m_line_events () + m_line_events (), + m_predecessors () { const char *line = location_get_source_line (filename, line_num, &m_len); @@ -606,7 +708,12 @@ edited_line::edited_line (const char *filename, int line_num) edited_line::~edited_line () { + unsigned i; + added_line *pred; + free (m_content); + FOR_EACH_VEC_ELT (m_predecessors, i, pred) + delete pred; } /* A callback for deleting edited_line *, for use as a @@ -643,6 +750,17 @@ edited_line::apply_fixit (int start_column, const char *replacement_str, int replacement_len) { + /* Handle newlines. They will only ever be at the end of the + replacement text, thanks to the filtering in rich_location. */ + if (replacement_len > 1) + if (replacement_str[replacement_len - 1] == '\n') + { + /* Stash in m_predecessors, stripping off newline. */ + m_predecessors.safe_push (new added_line (replacement_str, + replacement_len - 1)); + return true; + } + start_column = get_effective_column (start_column); next_column = get_effective_column (next_column); @@ -689,6 +807,57 @@ edited_line::apply_fixit (int start_column, return true; } +/* Determine the number of lines that will be present after + editing for this line. Typically this is just 1, but + if newlines have been added before this line, they will + also be counted. */ + +int +edited_line::get_effective_line_count () const +{ + return m_predecessors.length () + 1; +} + +/* Subroutine of edited_file::print_content. + Print this line and any new lines added before it, to PP. */ + +void +edited_line::print_content (pretty_printer *pp) const +{ + unsigned i; + added_line *pred; + FOR_EACH_VEC_ELT (m_predecessors, i, pred) + { + pp_string (pp, pred->get_content ()); + pp_newline (pp); + } + pp_string (pp, m_content); +} + +/* Subroutine of edited_file::print_run_of_changed_lines for + printing diff hunks to PP. + Print the '+' line for this line, and any newlines added + before it. + Note that if this edited_line was actually edited, the '-' + line has already been printed. If it wasn't, then we merely + have a placeholder edited_line for adding newlines to, and + we need to print a ' ' line for the edited_line as we haven't + printed it yet. */ + +void +edited_line::print_diff_lines (pretty_printer *pp) const +{ + unsigned i; + added_line *pred; + FOR_EACH_VEC_ELT (m_predecessors, i, pred) + print_diff_line (pp, '+', pred->get_content (), + pred->get_len ()); + if (actually_edited_p ()) + print_diff_line (pp, '+', m_content, m_len); + else + print_diff_line (pp, ' ', m_content, m_len); +} + /* Ensure that the buffer for m_content is at least large enough to hold a string of length LEN and its 0-terminator, doubling on repeated allocations. */ @@ -967,6 +1136,57 @@ test_applying_fixits_insert_after_failure (const line_table_case &case_) ASSERT_EQ (NULL, edit.generate_diff (false)); } +/* Test applying an "insert" fixit that adds a newline. */ + +static void +test_applying_fixits_insert_containing_newline (const line_table_case &case_) +{ + /* Create a tempfile and write some text to it. + .........................0000000001111111. + .........................1234567890123456. */ + const char *old_content = (" case 'a':\n" /* line 1. */ + " x = a;\n" /* line 2. */ + " case 'b':\n" /* line 3. */ + " x = b;\n");/* line 4. */ + + temp_source_file tmp (SELFTEST_LOCATION, ".c", old_content); + const char *filename = tmp.get_filename (); + line_table_test ltt (case_); + linemap_add (line_table, LC_ENTER, false, tmp.get_filename (), 3); + + /* Add a "break;" on a line by itself before line 3 i.e. before + column 1 of line 3. */ + location_t case_start = linemap_position_for_column (line_table, 5); + location_t case_finish = linemap_position_for_column (line_table, 13); + location_t case_loc = make_location (case_start, case_start, case_finish); + rich_location richloc (line_table, case_loc); + location_t line_start = linemap_position_for_column (line_table, 1); + richloc.add_fixit_insert_before (line_start, " break;\n"); + + if (case_finish > LINE_MAP_MAX_LOCATION_WITH_COLS) + return; + + edit_context edit; + edit.add_fixits (&richloc); + auto_free new_content = edit.get_content (filename); + ASSERT_STREQ ((" case 'a':\n" + " x = a;\n" + " break;\n" + " case 'b':\n" + " x = b;\n"), + new_content); + + /* Verify diff. */ + auto_free diff = edit.generate_diff (false); + ASSERT_STREQ (("@@ -1,4 +1,5 @@\n" + " case 'a':\n" + " x = a;\n" + "+ break;\n" + " case 'b':\n" + " x = b;\n"), + diff); +} + /* Test applying a "replace" fixit that grows the affected line. */ static void @@ -1057,6 +1277,44 @@ test_applying_fixits_shrinking_replace (const line_table_case &case_) } } +/* Replacement fix-it hint containing a newline. */ + +static void +test_applying_fixits_replace_containing_newline (const line_table_case &case_) +{ + /* Create a tempfile and write some text to it. + .........................0000000001111. + .........................1234567890123. */ + const char *old_content = "foo = bar ();\n"; + + temp_source_file tmp (SELFTEST_LOCATION, ".c", old_content); + const char *filename = tmp.get_filename (); + line_table_test ltt (case_); + linemap_add (line_table, LC_ENTER, false, filename, 1); + + /* Replace the " = " with "\n = ", as if we were reformatting an + overly long line. */ + location_t start = linemap_position_for_column (line_table, 4); + location_t finish = linemap_position_for_column (line_table, 6); + location_t loc = linemap_position_for_column (line_table, 13); + rich_location richloc (line_table, loc); + source_range range = source_range::from_locations (start, finish); + richloc.add_fixit_replace (range, "\n = "); + + /* Newlines are only supported within fix-it hints that + are at the start of lines (for entirely new lines), hence + this fix-it should not be displayed. */ + ASSERT_TRUE (richloc.seen_impossible_fixit_p ()); + + if (finish > LINE_MAP_MAX_LOCATION_WITH_COLS) + return; + + edit_context edit; + edit.add_fixits (&richloc); + auto_free new_content = edit.get_content (filename); + //ASSERT_STREQ ("foo\n = bar ();\n", new_content); +} + /* Test applying a "remove" fixit. */ static void @@ -1204,6 +1462,32 @@ change_line (edit_context &edit, int line_num) return loc; } +/* Subroutine of test_applying_fixits_multiple_lines. + Add the text "INSERTED\n" in front of the given line. */ + +static location_t +insert_line (edit_context &edit, int line_num) +{ + const line_map_ordinary *ord_map + = LINEMAPS_LAST_ORDINARY_MAP (line_table); + const int column = 1; + location_t loc = + linemap_position_for_line_and_column (line_table, ord_map, + line_num, column); + + expanded_location exploc = expand_location (loc); + if (loc <= LINE_MAP_MAX_LOCATION_WITH_COLS) + { + ASSERT_EQ (line_num, exploc.line); + ASSERT_EQ (column, exploc.column); + } + + rich_location insert (line_table, loc); + insert.add_fixit_insert_before ("INSERTED\n"); + edit.add_fixits (&insert); + return loc; +} + /* Test of editing multiple lines within a long file, to ensure that diffs are generated as expected. */ @@ -1229,6 +1513,7 @@ test_applying_fixits_multiple_lines (const line_table_case &case_) change_line (edit, 2); change_line (edit, 3); change_line (edit, 4); + insert_line (edit, 5); /* A run of nearby lines, within the contextual limit. */ change_line (edit, 150); @@ -1240,7 +1525,7 @@ test_applying_fixits_multiple_lines (const line_table_case &case_) /* Verify diff. */ auto_free diff = edit.generate_diff (false); - ASSERT_STREQ ("@@ -1,7 +1,7 @@\n" + ASSERT_STREQ ("@@ -1,7 +1,8 @@\n" " line 1\n" "-line 2\n" "-line 3\n" @@ -1248,10 +1533,11 @@ test_applying_fixits_multiple_lines (const line_table_case &case_) "+CHANGED: line 2\n" "+CHANGED: line 3\n" "+CHANGED: line 4\n" + "+INSERTED\n" " line 5\n" " line 6\n" " line 7\n" - "@@ -147,10 +147,10 @@\n" + "@@ -147,10 +148,10 @@\n" " line 147\n" " line 148\n" " line 149\n" @@ -1510,8 +1796,10 @@ edit_context_c_tests () for_each_line_table_case (test_applying_fixits_insert_after); for_each_line_table_case (test_applying_fixits_insert_after_at_line_end); for_each_line_table_case (test_applying_fixits_insert_after_failure); + for_each_line_table_case (test_applying_fixits_insert_containing_newline); for_each_line_table_case (test_applying_fixits_growing_replace); for_each_line_table_case (test_applying_fixits_shrinking_replace); + for_each_line_table_case (test_applying_fixits_replace_containing_newline); for_each_line_table_case (test_applying_fixits_remove); for_each_line_table_case (test_applying_fixits_multiple); for_each_line_table_case (test_applying_fixits_multiple_lines); diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index 4a7c4e44a6b..33a5cde0ab1 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,16 @@ +2017-05-02 David Malcolm + + * gcc.dg/plugin/diagnostic-test-show-locus-bw.c + (test_fixit_insert_newline): New function. + * gcc.dg/plugin/diagnostic-test-show-locus-color.c + (test_fixit_insert_newline): New function. + * gcc.dg/plugin/diagnostic-test-show-locus-generate-patch.c + (test_fixit_insert_newline): New function. + * gcc.dg/plugin/diagnostic-test-show-locus-parseable-fixits.c + (test_fixit_insert_newline): New function. + * gcc.dg/plugin/diagnostic_plugin_test_show_locus.c + (test_show_locus): Handle test_fixit_insert_newline. + 2017-05-02 Bin Cheng * g++.dg/tree-ssa/ivopts-3.C: Adjust test string. diff --git a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-bw.c b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-bw.c index e8112bfa3a5..25b9d6cb382 100644 --- a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-bw.c +++ b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-bw.c @@ -249,3 +249,23 @@ void test_many_nested_locations (void) MOLLIT ANIM ID EST LABORUM { dg-end-multiline-output "" } */ } + +/* Unit test for rendering of fix-it hints that add new lines. */ + +void test_fixit_insert_newline (void) +{ +#if 0 + switch (op) + { + case 'a': + x = a; + case 'b': /* { dg-warning "newline insertion" } */ + x = b; + } +/* { dg-begin-multiline-output "" } ++ break; + case 'b': + ^~~~~~~~ + { dg-end-multiline-output "" } */ +#endif +} diff --git a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-color.c b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-color.c index 712375e0b19..3fb1c240238 100644 --- a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-color.c +++ b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-color.c @@ -193,3 +193,23 @@ void test_fixit_replace (void) { dg-end-multiline-output "" } */ #endif } + +/* Unit test for rendering of fix-it hints that add new lines. */ + +void test_fixit_insert_newline (void) +{ +#if 0 + switch (op) + { + case 'a': + x = a; + case 'b': /* { dg-warning "newline insertion" } */ + x = b; + } +/* { dg-begin-multiline-output "" } ++ break; + case 'b': + ^~~~~~~~ + { dg-end-multiline-output "" } */ +#endif +} diff --git a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-generate-patch.c b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-generate-patch.c index afbaf635401..4522dcd350e 100644 --- a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-generate-patch.c +++ b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-generate-patch.c @@ -36,7 +36,20 @@ void test_fixit_replace (void) #endif } +/* Unit test for rendering of fix-it hints that add new lines. */ +void test_fixit_insert_newline (void) +{ +#if 0 + switch (op) + { + case 'a': + x = a; + case 'b': /* { dg-warning "newline insertion" } */ + x = b; + } +#endif +} /* Verify the output from -fdiagnostics-generate-patch. We expect a header, containing the filename. This is the absolute path, @@ -74,4 +87,12 @@ void test_fixit_replace (void) #endif } +@@ -45,6 +45,7 @@ + { + case 'a': + x = a; ++ break; + case 'b': + x = b; + } { dg-end-multiline-output "" } */ diff --git a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-parseable-fixits.c b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-parseable-fixits.c index 1490c981e45..25a7c3c5072 100644 --- a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-parseable-fixits.c +++ b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-show-locus-parseable-fixits.c @@ -39,3 +39,19 @@ void test_fixit_replace (void) /* { dg-regexp "fix-it:.*\\{38:3-38:21\\}:.*" } */ #endif } + +/* Unit test for rendering of fix-it hints that add new lines. */ + +void test_fixit_insert_newline (void) +{ +#if 0 + switch (op) + { + case 'a': + x = a; + case 'b': /* { dg-warning "newline insertion" } */ + x = b; + } +/* { dg-regexp "fix-it:.*\\{52:1-52:1\\}:.*\\n" } */ +#endif +} diff --git a/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_test_show_locus.c b/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_test_show_locus.c index 3efc7dfa0b4..6afb58414f0 100644 --- a/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_test_show_locus.c +++ b/gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_test_show_locus.c @@ -268,6 +268,18 @@ test_show_locus (function *fun) warning_at_rich_loc (&richloc, 0, "example of insertion hints"); } + if (0 == strcmp (fnname, "test_fixit_insert_newline")) + { + const int line = fnstart_line + 6; + location_t line_start = get_loc (line, 0); + location_t case_start = get_loc (line, 4); + location_t case_finish = get_loc (line, 11); + location_t case_loc = make_location (case_start, case_start, case_finish); + rich_location richloc (line_table, case_loc); + richloc.add_fixit_insert_before (line_start, " break;\n"); + warning_at_rich_loc (&richloc, 0, "example of newline insertion hint"); + } + if (0 == strcmp (fnname, "test_fixit_remove")) { const int line = fnstart_line + 2; diff --git a/libcpp/ChangeLog b/libcpp/ChangeLog index ab225d53519..c01ab44d3f7 100644 --- a/libcpp/ChangeLog +++ b/libcpp/ChangeLog @@ -1,3 +1,14 @@ +2017-05-02 David Malcolm + + * include/line-map.h (class rich_location): Update description of + newline handling. + (class fixit_hint): Likewise. + (fixit_hint::ends_with_newline_p): New decl. + * line-map.c (rich_location::maybe_add_fixit): Support newlines + in fix-it hints that are insertions of single lines at the start + of a line. Don't consolidate into such fix-it hints. + (fixit_hint::ends_with_newline_p): New method. + 2017-05-01 David Malcolm * include/line-map.h (source_range::intersects_line_p): Delete. diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h index 0c44f01b3bd..90d65d746ab 100644 --- a/libcpp/include/line-map.h +++ b/libcpp/include/line-map.h @@ -1551,7 +1551,11 @@ class fixit_hint; Attempts to add a fix-it hint within a macro expansion will fail. - We do not yet support newlines in fix-it text; attempts to do so will fail. + There is only limited support for newline characters in fix-it hints: + only hints with newlines which insert an entire new line are permitted, + inserting at the start of a line, and finishing with a newline + (with no interior newline characters). Other attempts to add + fix-it hints containing newline characters will fail. The rich_location API handles these failures gracefully, so that diagnostics can attempt to add fix-it hints without each needing @@ -1690,7 +1694,13 @@ protected: [start, next_loc) Insertions have start == next_loc: "replace" the empty string at the start location with the new string. - Deletions are replacement with the empty string. */ + Deletions are replacement with the empty string. + + There is only limited support for newline characters in fix-it hints + as noted above in the comment for class rich_location. + A fixit_hint instance can have at most one newline character; if + present, the newline character must be the final character of + the content (preventing e.g. fix-its that split a pre-existing line). */ class fixit_hint { @@ -1712,6 +1722,8 @@ class fixit_hint bool insertion_p () const { return m_start == m_next_loc; } + bool ends_with_newline_p () const; + private: /* We don't use source_range here since, unlike most places, this is a half-open/half-closed range: diff --git a/libcpp/line-map.c b/libcpp/line-map.c index 176e58d2545..c4b7cb23651 100644 --- a/libcpp/line-map.c +++ b/libcpp/line-map.c @@ -2325,16 +2325,44 @@ rich_location::maybe_add_fixit (source_location start, if (reject_impossible_fixit (next_loc)) return; - /* We do not yet support newlines within fix-it hints. */ - if (strchr (new_content, '\n')) + const char *newline = strchr (new_content, '\n'); + if (newline) { - stop_supporting_fixits (); - return; + /* For now, we can only support insertion of whole lines + i.e. starts at start of line, and the newline is at the end of + the insertion point. */ + + /* It must be an insertion, not a replacement/deletion. */ + if (start != next_loc) + { + stop_supporting_fixits (); + return; + } + + /* The insertion must be at the start of a line. */ + expanded_location exploc_start + = linemap_client_expand_location_to_spelling_point (start); + if (exploc_start.column != 1) + { + stop_supporting_fixits (); + return; + } + + /* The newline must be at end of NEW_CONTENT. + We could eventually split up fix-its at newlines if we wanted + to allow more generality (e.g. to allow adding multiple lines + with one add_fixit call. */ + if (newline[1] != '\0') + { + stop_supporting_fixits (); + return; + } } - /* Consolidate neighboring fixits. */ + /* Consolidate neighboring fixits. + Don't consolidate into newline-insertion fixits. */ fixit_hint *prev = get_last_fixit_hint (); - if (prev) + if (prev && !prev->ends_with_newline_p ()) if (prev->maybe_append (start, next_loc, new_content)) return; @@ -2398,3 +2426,13 @@ fixit_hint::maybe_append (source_location start, m_bytes[m_len] = '\0'; return true; } + +/* Return true iff this hint's content ends with a newline. */ + +bool +fixit_hint::ends_with_newline_p () const +{ + if (m_len == 0) + return false; + return m_bytes[m_len - 1] == '\n'; +} -- 2.30.2