The first aspect of this patch is to add some checking of fix-it hints.
The idea is to put this checking within the rich_location machinery,
rather than requiring every diagnostic to implement it for itself.
The fixits within a rich_location are "atomic": all must be valid for
any to be applicable.
We reject any fixits involving locations above
LINE_MAP_MAX_LOCATION_WITH_COLS.
There's no guarantee that it's sane to modify a macro, so we reject
any fix-its that touch them.
For example, note the attempt to provide a fix-it for the definition
of the macro FIELD:
spellcheck-fields-2.c: In function ‘test_macro’:
spellcheck-fields-2.c:26:15: error: ‘union u’ has no member named ‘colour’; did you mean ‘color’?
#define FIELD colour
^
color
spellcheck-fields-2.c:27:15: note: in expansion of macro ‘FIELD’
return ptr->FIELD;
^~~~~
After this patch, the fixit is not displayed:
spellcheck-fields-2.c: In function ‘test_macro’:
spellcheck-fields-2.c:26:15: error: ‘union u’ has no member named ‘colour’; did you mean ‘color’?
#define FIELD colour
^
spellcheck-fields-2.c:27:15: note: in expansion of macro ‘FIELD’
return ptr->FIELD;
^~~~~
We might want some way for a diagnostic to opt-in to fix-its that
affect macros, but for now it's simplest to reject them.
The other aspect of this patch is fix-it consolidation: in some cases
neighboring fix-its can be merged. For example, in a diagnostic to
modernize old-style struct initializers from:
struct s example = {
- foo: 1,
+ .foo = 1,
};
one approach would be to replace the "foo" with ".foo" and the ":"
with " =". This would give two "replace" fix-its:
foo: 1,
--- FIXIT 1
.foo
- FIXIT 2
=
This patch allows them to be consolidated into a single "replace" fix-it:
foo: 1,
----
.foo =
gcc/ChangeLog:
* diagnostic-show-locus.c
(selftest::test_fixit_consolidation): New function.
(selftest::diagnostic_show_locus_c_tests): Call it.
* gcc-rich-location.h (gcc_rich_location): Eliminate unused
constructor based on source_range.
gcc/testsuite/ChangeLog:
* gcc.dg/spellcheck-fields-2.c (test): Move
dg-begin/end-multiline-output within function body.
(test_macro): New function.
libcpp/ChangeLog:
* include/line-map.h (rich_location): Eliminate unimplemented
constructor based on source_range.
(rich_location::get_last_fixit_hint): New method.
(rich_location::reject_impossible_fixit): New method.
(rich_location): Add fields m_line_table and
m_seen_impossible_fixit.
(fixit_hint::maybe_append_replace): New pure virtual function.
(fixit_insert::maybe_append_replace): New function.
(fixit_replace::maybe_append_replace): New function.
* line-map.c (rich_location::rich_location): Initialize
m_line_table and m_seen_impossible_fixit.
(rich_location::add_fixit_insert): Call
reject_impossible_fixit and bail out if true.
(column_before_p): New function.
(rich_location::add_fixit_replace): Call reject_impossible_fixit
and bail out if true. Attempt to consolidate with neighboring
fixits.
(rich_location::get_last_fixit_hint): New method.
(rich_location::reject_impossible_fixit): New method.
(fixit_insert::maybe_append_replace): New method.
(fixit_replace::maybe_append_replace): New method.
From-SVN: r239789
+2016-08-26 David Malcolm <dmalcolm@redhat.com>
+
+ * diagnostic-show-locus.c
+ (selftest::test_fixit_consolidation): New function.
+ (selftest::diagnostic_show_locus_c_tests): Call it.
+ * gcc-rich-location.h (gcc_rich_location): Eliminate unused
+ constructor based on source_range.
+
2016-08-26 David Malcolm <dmalcolm@redhat.com>
* diagnostic-color.c (color_dict): Add "fixit-insert" and
test_one_liner_fixit_replace_equal_secondary_range ();
}
+/* Verify that fix-it hints are appropriately consolidated.
+
+ If any fix-it hints in a rich_location involve locations beyond
+ LINE_MAP_MAX_LOCATION_WITH_COLS, then we can't reliably apply
+ the fix-it as a whole, so there should be none.
+
+ Otherwise, verify that consecutive "replace" and "remove" fix-its
+ are merged, and that other fix-its remain separate. */
+
+static void
+test_fixit_consolidation (const line_table_case &case_)
+{
+ line_table_test ltt (case_);
+
+ linemap_add (line_table, LC_ENTER, false, "test.c", 1);
+
+ const location_t c10 = linemap_position_for_column (line_table, 10);
+ const location_t c15 = linemap_position_for_column (line_table, 15);
+ const location_t c16 = linemap_position_for_column (line_table, 16);
+ const location_t c17 = linemap_position_for_column (line_table, 17);
+ const location_t c20 = linemap_position_for_column (line_table, 20);
+ const location_t caret = c10;
+
+ /* Insert + insert. */
+ {
+ rich_location richloc (line_table, caret);
+ richloc.add_fixit_insert (c10, "foo");
+ richloc.add_fixit_insert (c15, "bar");
+
+ if (c15 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+ /* Bogus column info for 2nd fixit, so no fixits. */
+ ASSERT_EQ (0, richloc.get_num_fixit_hints ());
+ else
+ /* They should not have been merged. */
+ ASSERT_EQ (2, richloc.get_num_fixit_hints ());
+ }
+
+ /* Insert + replace. */
+ {
+ rich_location richloc (line_table, caret);
+ richloc.add_fixit_insert (c10, "foo");
+ richloc.add_fixit_replace (source_range::from_locations (c15, c17),
+ "bar");
+
+ if (c17 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+ /* Bogus column info for 2nd fixit, so no fixits. */
+ ASSERT_EQ (0, richloc.get_num_fixit_hints ());
+ else
+ /* They should not have been merged. */
+ ASSERT_EQ (2, richloc.get_num_fixit_hints ());
+ }
+
+ /* Replace + non-consecutive insert. */
+ {
+ rich_location richloc (line_table, caret);
+ richloc.add_fixit_replace (source_range::from_locations (c10, c15),
+ "bar");
+ richloc.add_fixit_insert (c17, "foo");
+
+ if (c17 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+ /* Bogus column info for 2nd fixit, so no fixits. */
+ ASSERT_EQ (0, richloc.get_num_fixit_hints ());
+ else
+ /* They should not have been merged. */
+ ASSERT_EQ (2, richloc.get_num_fixit_hints ());
+ }
+
+ /* Replace + non-consecutive replace. */
+ {
+ rich_location richloc (line_table, caret);
+ richloc.add_fixit_replace (source_range::from_locations (c10, c15),
+ "foo");
+ richloc.add_fixit_replace (source_range::from_locations (c17, c20),
+ "bar");
+
+ if (c20 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+ /* Bogus column info for 2nd fixit, so no fixits. */
+ ASSERT_EQ (0, richloc.get_num_fixit_hints ());
+ else
+ /* They should not have been merged. */
+ ASSERT_EQ (2, richloc.get_num_fixit_hints ());
+ }
+
+ /* Replace + consecutive replace. */
+ {
+ rich_location richloc (line_table, caret);
+ richloc.add_fixit_replace (source_range::from_locations (c10, c15),
+ "foo");
+ richloc.add_fixit_replace (source_range::from_locations (c16, c20),
+ "bar");
+
+ if (c20 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+ /* Bogus column info for 2nd fixit, so no fixits. */
+ ASSERT_EQ (0, richloc.get_num_fixit_hints ());
+ else
+ {
+ /* They should have been merged into a single "replace". */
+ ASSERT_EQ (1, richloc.get_num_fixit_hints ());
+ const fixit_hint *hint = richloc.get_fixit_hint (0);
+ ASSERT_EQ (fixit_hint::REPLACE, hint->get_kind ());
+ const fixit_replace *replace = (const fixit_replace *)hint;
+ ASSERT_STREQ ("foobar", replace->get_string ());
+ ASSERT_EQ (c10, replace->get_range ().m_start);
+ ASSERT_EQ (c20, replace->get_range ().m_finish);
+ }
+ }
+
+ /* Replace + consecutive removal. */
+ {
+ rich_location richloc (line_table, caret);
+ richloc.add_fixit_replace (source_range::from_locations (c10, c15),
+ "foo");
+ richloc.add_fixit_remove (source_range::from_locations (c16, c20));
+
+ if (c20 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+ /* Bogus column info for 2nd fixit, so no fixits. */
+ ASSERT_EQ (0, richloc.get_num_fixit_hints ());
+ else
+ {
+ /* They should have been merged into a single replace, with the
+ range extended to cover that of the removal. */
+ ASSERT_EQ (1, richloc.get_num_fixit_hints ());
+ const fixit_hint *hint = richloc.get_fixit_hint (0);
+ ASSERT_EQ (fixit_hint::REPLACE, hint->get_kind ());
+ const fixit_replace *replace = (const fixit_replace *)hint;
+ ASSERT_STREQ ("foo", replace->get_string ());
+ ASSERT_EQ (c10, replace->get_range ().m_start);
+ ASSERT_EQ (c20, replace->get_range ().m_finish);
+ }
+ }
+
+ /* Consecutive removals. */
+ {
+ rich_location richloc (line_table, caret);
+ richloc.add_fixit_remove (source_range::from_locations (c10, c15));
+ richloc.add_fixit_remove (source_range::from_locations (c16, c20));
+
+ if (c20 > LINE_MAP_MAX_LOCATION_WITH_COLS)
+ /* Bogus column info for 2nd fixit, so no fixits. */
+ ASSERT_EQ (0, richloc.get_num_fixit_hints ());
+ else
+ {
+ /* They should have been merged into a single "replace-with-empty". */
+ ASSERT_EQ (1, richloc.get_num_fixit_hints ());
+ const fixit_hint *hint = richloc.get_fixit_hint (0);
+ ASSERT_EQ (fixit_hint::REPLACE, hint->get_kind ());
+ const fixit_replace *replace = (const fixit_replace *)hint;
+ ASSERT_STREQ ("", replace->get_string ());
+ ASSERT_EQ (c10, replace->get_range ().m_start);
+ ASSERT_EQ (c20, replace->get_range ().m_finish);
+ }
+ }
+}
+
/* Run all of the selftests within this file. */
void
test_diagnostic_show_locus_unknown_location ();
for_each_line_table_case (test_diagnostic_show_locus_one_liner);
+ for_each_line_table_case (test_fixit_consolidation);
}
} // namespace selftest
gcc_rich_location (source_location loc) :
rich_location (line_table, loc) {}
- /* Constructing from a source_range. */
- gcc_rich_location (source_range src_range) :
- rich_location (src_range) {}
-
-
/* Methods for adding ranges via gcc entities. */
void
add_expr (tree expr);
+2016-08-26 David Malcolm <dmalcolm@redhat.com>
+
+ * gcc.dg/spellcheck-fields-2.c (test): Move
+ dg-begin/end-multiline-output within function body.
+ (test_macro): New function.
+
2016-08-26 David Malcolm <dmalcolm@redhat.com>
* gcc.dg/plugin/diagnostic-test-show-locus-color.c
int test (union u *ptr)
{
return ptr->colour; /* { dg-error "did you mean .color.?" } */
-}
/* Verify that we get an underline and a fixit hint. */
/* { dg-begin-multiline-output "" }
^~~~~~
color
{ dg-end-multiline-output "" } */
+}
+
+
+/* Verify that we don't offer a fixit hint in the presence of
+ a macro. */
+int test_macro (union u *ptr)
+{
+#define FIELD colour /* { dg-error "did you mean .color.?" } */
+ return ptr->FIELD;
+
+/* { dg-begin-multiline-output "" }
+ #define FIELD colour
+ ^
+ { dg-end-multiline-output "" } */
+
+/* { dg-begin-multiline-output "" }
+ return ptr->FIELD;
+ ^~~~~
+ { dg-end-multiline-output "" } */
+
+#undef FIELD
+}
+2016-08-26 David Malcolm <dmalcolm@redhat.com>
+
+ * include/line-map.h (rich_location): Eliminate unimplemented
+ constructor based on source_range.
+ (rich_location::get_last_fixit_hint): New method.
+ (rich_location::reject_impossible_fixit): New method.
+ (rich_location): Add fields m_line_table and
+ m_seen_impossible_fixit.
+ (fixit_hint::maybe_append_replace): New pure virtual function.
+ (fixit_insert::maybe_append_replace): New function.
+ (fixit_replace::maybe_append_replace): New function.
+ * line-map.c (rich_location::rich_location): Initialize
+ m_line_table and m_seen_impossible_fixit.
+ (rich_location::add_fixit_insert): Call
+ reject_impossible_fixit and bail out if true.
+ (column_before_p): New function.
+ (rich_location::add_fixit_replace): Call reject_impossible_fixit
+ and bail out if true. Attempt to consolidate with neighboring
+ fixits.
+ (rich_location::get_last_fixit_hint): New method.
+ (rich_location::reject_impossible_fixit): New method.
+ (fixit_insert::maybe_append_replace): New method.
+ (fixit_replace::maybe_append_replace): New method.
+
2016-08-23 David Malcolm <dmalcolm@redhat.com>
* include/line-map.h (source_range::from_locations): New method.
/* Constructing from a location. */
rich_location (line_maps *set, source_location loc);
- /* Constructing from a source_range. */
- rich_location (source_range src_range);
-
/* Destructor. */
~rich_location ();
unsigned int get_num_fixit_hints () const { return m_num_fixit_hints; }
fixit_hint *get_fixit_hint (int idx) const { return m_fixit_hints[idx]; }
+ fixit_hint *get_last_fixit_hint () const;
+
+private:
+ bool reject_impossible_fixit (source_location where);
public:
static const int MAX_RANGES = 3;
static const int MAX_FIXIT_HINTS = 2;
protected:
+ line_maps *m_line_table;
unsigned int m_num_ranges;
location_range m_ranges[MAX_RANGES];
unsigned int m_num_fixit_hints;
fixit_hint *m_fixit_hints[MAX_FIXIT_HINTS];
+ bool m_seen_impossible_fixit;
};
class fixit_hint
virtual bool affects_line_p (const char *file, int line) = 0;
virtual source_location get_start_loc () const = 0;
virtual bool maybe_get_end_loc (source_location *out) const = 0;
+ /* Vfunc for consolidating successor fixits. */
+ virtual bool maybe_append_replace (line_maps *set,
+ source_range src_range,
+ const char *new_content) = 0;
};
class fixit_insert : public fixit_hint
bool affects_line_p (const char *file, int line);
source_location get_start_loc () const { return m_where; }
bool maybe_get_end_loc (source_location *) const { return false; }
+ bool maybe_append_replace (line_maps *set,
+ source_range src_range,
+ const char *new_content);
source_location get_location () const { return m_where; }
const char *get_string () const { return m_bytes; }
*out = m_src_range.m_finish;
return true;
}
+ bool maybe_append_replace (line_maps *set,
+ source_range src_range,
+ const char *new_content);
source_range get_range () const { return m_src_range; }
const char *get_string () const { return m_bytes; }
/* Construct a rich_location with location LOC as its initial range. */
-rich_location::rich_location (line_maps */*set*/, source_location loc) :
+rich_location::rich_location (line_maps *set, source_location loc) :
+ m_line_table (set),
m_num_ranges (0),
m_column_override (0),
m_have_expanded_location (false),
- m_num_fixit_hints (0)
+ m_num_fixit_hints (0),
+ m_seen_impossible_fixit (false)
{
add_range (loc, true);
}
rich_location::add_fixit_insert (source_location where,
const char *new_content)
{
+ if (reject_impossible_fixit (where))
+ return;
+
linemap_assert (m_num_fixit_hints < MAX_FIXIT_HINTS);
m_fixit_hints[m_num_fixit_hints++]
= new fixit_insert (where, new_content);
add_fixit_replace (src_range, "");
}
+/* Return true iff A is in the column directly before B, on the
+ same line of the same source file. */
+
+static bool
+column_before_p (line_maps *set, source_location a, source_location b)
+{
+ if (IS_ADHOC_LOC (a))
+ a = get_location_from_adhoc_loc (set, a);
+ if (IS_ADHOC_LOC (b))
+ b = get_location_from_adhoc_loc (set, b);
+
+ /* They must both be in ordinary maps. */
+ const struct line_map *linemap_a = linemap_lookup (set, a);
+ if (linemap_macro_expansion_map_p (linemap_a))
+ return false;
+ const struct line_map *linemap_b = linemap_lookup (set, b);
+ if (linemap_macro_expansion_map_p (linemap_b))
+ return false;
+
+ /* To be on the same line, they must be in the same ordinary map. */
+ if (linemap_a != linemap_b)
+ return false;
+
+ linenum_type line_a
+ = SOURCE_LINE (linemap_check_ordinary (linemap_a), a);
+ linenum_type line_b
+ = SOURCE_LINE (linemap_check_ordinary (linemap_b), b);
+ if (line_a != line_b)
+ return false;
+
+ linenum_type column_a
+ = SOURCE_COLUMN (linemap_check_ordinary (linemap_a), a);
+ linenum_type column_b
+ = SOURCE_COLUMN (linemap_check_ordinary (linemap_b), b);
+
+ return column_b == column_a + 1;
+}
+
/* Add a fixit-hint, suggesting replacement of the content at
SRC_RANGE with NEW_CONTENT. */
const char *new_content)
{
linemap_assert (m_num_fixit_hints < MAX_FIXIT_HINTS);
+
+ if (reject_impossible_fixit (src_range.m_start))
+ return;
+ if (reject_impossible_fixit (src_range.m_finish))
+ return;
+
+ /* Consolidate neighboring fixits. */
+ fixit_hint *prev = get_last_fixit_hint ();
+ if (m_num_fixit_hints > 0)
+ {
+ if (prev->maybe_append_replace (m_line_table, src_range, new_content))
+ return;
+ }
+
m_fixit_hints[m_num_fixit_hints++]
= new fixit_replace (src_range, new_content);
}
+/* Get the last fix-it hint within this rich_location, or NULL if none. */
+
+fixit_hint *
+rich_location::get_last_fixit_hint () const
+{
+ if (m_num_fixit_hints > 0)
+ return m_fixit_hints[m_num_fixit_hints - 1];
+ else
+ return NULL;
+}
+
+/* If WHERE is an "awkward" location, then mark this rich_location as not
+ supporting fixits, purging any thay were already added, and return true.
+
+ Otherwise (the common case), return false. */
+
+bool
+rich_location::reject_impossible_fixit (source_location where)
+{
+ /* Fix-its within a rich_location should either all be suggested, or
+ none of them should be suggested.
+ Once we've rejected a fixit, we reject any more, even those
+ with reasonable locations. */
+ if (m_seen_impossible_fixit)
+ return true;
+
+ if (where <= LINE_MAP_MAX_LOCATION_WITH_COLS)
+ /* WHERE is a reasonable location for a fix-it; don't reject it. */
+ return false;
+
+ /* Otherwise we have an attempt to add a fix-it with an "awkward"
+ location: either one that we can't obtain column information
+ for (within an ordinary map), or one within a macro expansion. */
+ m_seen_impossible_fixit = true;
+
+ /* Purge the rich_location of any fix-its that were already added. */
+ for (unsigned int i = 0; i < m_num_fixit_hints; i++)
+ delete m_fixit_hints[i];
+ m_num_fixit_hints = 0;
+
+ return true;
+}
+
/* class fixit_insert. */
fixit_insert::fixit_insert (source_location where,
return false;
}
+/* Implementation of maybe_append_replace for fixit_insert. Reject
+ the attempt to consolidate fix-its. */
+
+bool
+fixit_insert::maybe_append_replace (line_maps *, source_range, const char *)
+{
+ return false;
+}
+
/* class fixit_replace. */
fixit_replace::fixit_replace (source_range src_range,
{
return m_src_range.intersects_line_p (file, line);
}
+
+/* Implementation of maybe_append_replace for fixit_replace. If
+ possible, merge the new replacement into this one and return true.
+ Otherwise return false. */
+
+bool
+fixit_replace::maybe_append_replace (line_maps *set,
+ source_range src_range,
+ const char *new_content)
+{
+ /* Does SRC_RANGE start immediately after this one finishes? */
+ if (!column_before_p (set, m_src_range.m_finish, src_range.m_start))
+ return false;
+
+ /* We have neighboring replacements; merge them. */
+ m_src_range.m_finish = src_range.m_finish;
+ size_t extra_len = strlen (new_content);
+ m_bytes = (char *)xrealloc (m_bytes, m_len + extra_len + 1);
+ memcpy (m_bytes + m_len, new_content, extra_len);
+ m_len += extra_len;
+ m_bytes[m_len] = '\0';
+ return true;
+}
projects / gcc.git / commitdiff