On Fri, Aug 11, 2023 at 07:02:49PM -0400, David Malcolm wrote:
> On Wed, 2023-08-09 at 18:14 -0400, Lewis Hyatt wrote:
> > The previous patch in this series introduced the concept of LC_GEN line
> > maps. This patch continues on the path to using them to improve _Pragma
> > diagnostics, by adding a new source_id SRC member to struct
> > expanded_location, which is populated by linemap_expand_location. This
> > member allows call sites to detect and handle when a location refers to
> > generated data rather than a plain file name.
> >
> > The previous FILE member of expanded_location is preserved (although
> > redundant with SRC), so that call sites which do not and never will care
> > about generated data do not need to be concerned about it. Call sites that
> > will care are modified here, to use SRC rather than FILE for comparing
> > locations.
>
> Thanks; this seems like a good approach.
>
>
> [...snip...]
>
> > diff --git a/gcc/edit-context.cc b/gcc/edit-context.cc
> > index 6f5bc6b9d8f..15052aec417 100644
> > --- a/gcc/edit-context.cc
> > +++ b/gcc/edit-context.cc
> > @@ -295,7 +295,7 @@ edit_context::apply_fixit (const fixit_hint *hint)
> > {
> > expanded_location start = expand_location (hint->get_start_loc ());
> > expanded_location next_loc = expand_location (hint->get_next_loc ());
> > - if (start.file != next_loc.file)
> > + if (start.src != next_loc.src || start.src.is_buffer ())
> > return false;
> > if (start.line != next_loc.line)
> > return false;
>
> Thinking about fix-it hints, it makes sense to reject attempts to
> create fix-it hints within generated strings, as we can't apply them or
> visualize them.
>
> Does anywhere in the patch kit do that? Either of
> rich_location::maybe_add_fixit
> or
> rich_location::reject_impossible_fixit
> would be good places to do that.
>
So rich_location::reject_impossible_fixit does reject them for _Pragmas now,
because what the frontend sees and passes to it is a virtual location, and it
always rejects virtual locations. But it doesn't reject arbitrary generated
data locations that may be created in an ordinary non-virtual location. I
think it's this one-line change to reject those:
-- >8 --
diff --git a/libcpp/line-map.cc b/libcpp/line-map.cc
index 835e8e1b8cd..382594637ad 100644
--- a/libcpp/line-map.cc
+++ b/libcpp/line-map.cc
@@ -2545,7 +2545,8 @@ rich_location::maybe_add_fixit (location_t start,
= linemap_client_expand_location_to_spelling_point (next_loc,
LOCATION_ASPECT_START);
/* They must be within the same file... */
- if (exploc_start.src != exploc_next_loc.src)
+ if (exploc_start.src != exploc_next_loc.src
+ || exploc_start.src.is_buffer ())
{
stop_supporting_fixits ();
return;
-- >8 --
However, there are many selftests in diagnostic-show-locus.cc that actually
verify we generate the fixit hints for generated data, so I would need also to
change those to skip the test in this case as well. That looks like this:
-- >8 --
diff --git a/gcc/diagnostic-show-locus.cc b/gcc/diagnostic-show-locus.cc
index 62c60645e88..884c55e91e9 100644
--- a/gcc/diagnostic-show-locus.cc
+++ b/gcc/diagnostic-show-locus.cc
@@ -3824,6 +3824,8 @@ test_diagnostic_show_locus_one_liner (const
line_table_case &case_)
test_one_liner_simple_caret ();
test_one_liner_caret_and_range ();
test_one_liner_multiple_carets_and_ranges ();
+ if (!ltt.m_generated_data)
+ {
test_one_liner_fixit_insert_before ();
test_one_liner_fixit_insert_after ();
test_one_liner_fixit_remove ();
@@ -3835,6 +3837,7 @@ test_diagnostic_show_locus_one_liner (const
line_table_case &case_)
test_one_liner_many_fixits_2 ();
test_one_liner_labels ();
}
+}
/* Version of all one-liner tests exercising multibyte awareness. For
simplicity we stick to using two multibyte characters in the test, U+1F602
@@ -4419,6 +4422,8 @@ test_diagnostic_show_locus_one_liner_utf8 (const
line_table_case &case_)
test_one_liner_simple_caret_utf8 ();
test_one_liner_caret_and_range_utf8 ();
test_one_liner_multiple_carets_and_ranges_utf8 ();
+ if (!ltt.m_generated_data)
+ {
test_one_liner_fixit_insert_before_utf8 ();
test_one_liner_fixit_insert_after_utf8 ();
test_one_liner_fixit_remove_utf8 ();
@@ -4428,6 +4433,7 @@ test_diagnostic_show_locus_one_liner_utf8 (const
line_table_case &case_)
test_one_liner_fixit_validation_adhoc_locations_utf8 ();
test_one_liner_many_fixits_1_utf8 ();
test_one_liner_many_fixits_2_utf8 ();
+ }
test_one_liner_labels_utf8 ();
test_one_liner_colorized_utf8 ();
}
@@ -5726,15 +5732,15 @@ diagnostic_show_locus_cc_tests ()
for_each_line_table_case (test_diagnostic_show_locus_one_liner, true);
for_each_line_table_case (test_diagnostic_show_locus_one_liner_utf8, true);
for_each_line_table_case (test_add_location_if_nearby, true);
- for_each_line_table_case (test_diagnostic_show_locus_fixit_lines, true);
- for_each_line_table_case (test_fixit_consolidation, true);
- for_each_line_table_case (test_overlapped_fixit_printing, true);
- for_each_line_table_case (test_overlapped_fixit_printing_utf8, true);
- for_each_line_table_case (test_overlapped_fixit_printing_2, true);
- for_each_line_table_case (test_fixit_insert_containing_newline, true);
- for_each_line_table_case (test_fixit_insert_containing_newline_2, true);
- for_each_line_table_case (test_fixit_replace_containing_newline, true);
- for_each_line_table_case (test_fixit_deletion_affecting_newline, true);
+ for_each_line_table_case (test_diagnostic_show_locus_fixit_lines, false);
+ for_each_line_table_case (test_fixit_consolidation, false);
+ for_each_line_table_case (test_overlapped_fixit_printing, false);
+ for_each_line_table_case (test_overlapped_fixit_printing_utf8, false);
+ for_each_line_table_case (test_overlapped_fixit_printing_2, false);
+ for_each_line_table_case (test_fixit_insert_containing_newline, false);
+ for_each_line_table_case (test_fixit_insert_containing_newline_2, false);
+ for_each_line_table_case (test_fixit_replace_containing_newline, false);
+ for_each_line_table_case (test_fixit_deletion_affecting_newline, false);
for_each_line_table_case (test_tab_expansion, true);
for_each_line_table_case (test_escaping_bytes_1, true);
for_each_line_table_case (test_escaping_bytes_2, true);
-- >8 --
(The above diff was with -w to avoid a lot of useless indent changes, just for
illustration what it does.)
>
> [...snip...]
>
> > diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
> > index e59123b18c5..76617fe6129 100644
> > --- a/libcpp/include/line-map.h
> > +++ b/libcpp/include/line-map.h
> > @@ -1410,18 +1410,22 @@ linemap_location_before_p (class line_maps *set,
> >
> > typedef struct
> > {
> > - /* The name of the source file involved. */
> > - const char *file;
> > + /* The file name of the location involved, or NULL if the location
> > + is not in an external file. */
> > + const char *file = nullptr;
> >
> > - /* The line-location in the source file. */
> > - int line;
> > -
> > - int column;
> > + /* A source_id recording the file name and/or the in-memory content,
> > + as appropriate. Users that need to handle in-memory content need
> > + to use this rather than FILE. */
> > + source_id src;
> >
> > - void *data;
> > + /* The line-location in the source file. */
> > + int line = 0;
> > + int column = 0;
> > + void *data = nullptr;
> >
> > - /* In a system header?. */
> > - bool sysp;
> > + /* In a system header? */
> > + bool sysp = false;
> > } expanded_location;
>
> I don't know if we've been using default member initialization yet, but
> apparently it's C++11, and thus OK.
>
Thanks, I feel like it does make things more maintainable. FWIW, I did verify
it builds with gcc 4.8.5.
> [...snip...]
>
>
> This patch looks good to me, but obviously it has dependencies on the
> rest of the kit.
>
> Dave
>
Thank you, please let me know if I should also apply the above tweaks or no?
