Re: [PATCH v4 2/8] libcpp: diagnostics: Support generated data in expanded locations

[David Malcolm <dmalcolm@redhat.com>]

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.


[...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.

[...snip...]


This patch looks good to me, but obviously it has dependencies on the
rest of the kit.

Dave