Re: [PATCH] Add gnu::diagnose_as attribute

[David Malcolm <dmalcolm@redhat.com>]

On Tue, 2021-05-04 at 13:13 +0200, Matthias Kretz wrote:
> From: Matthias Kretz <kretz@kde.org>
> 
> This attribute overrides the diagnostics output string for the entity
> it
> appertains to. The motivation is to improve QoI for library TS
> implementations, where diagnostics have a very bad signal-to-noise
> ratio
> due to the long namespaces involved.
> 
> On Tuesday, 27 April 2021 11:46:48 CEST Jonathan Wakely wrote:
> > I think it's a great idea and would like to use it for all the TS
> > implementations where there is some inline namespace that the user
> > doesn't care about. std::experimental::fundamentals_v1:: would be
> > much
> > better as just std::experimental::, or something like std::[LFTS]::.
> 
> With the attribute, it is possible to solve PR89370 and make
> std::__cxx11::basic_string<_CharT, _Traits, _Alloc> appear as
> std::string in diagnostic output without extra hacks to recognize the
> type.

Thanks for the patch, it looks very promising.

The C++ frontend maintainers will need to review the C++ frontend parts
in detail, so I'll defer to them for the bulk of the review.

Various thoughts:

The patch has no testcases; it should probably add test coverage for:
- the various places and ways in which diagnose_as can affect the
output,
- disabling it with the option
- the various ways in which the user can get diagnose_as wrong
- etc

Does the patch affect the output of labels when underlining ranges of
source code in diagnostics?

Does the patch interact correctly with the %H and %I codes that try to
show the differences between two template types?

I have some minor nits from a diagnostics point of view:

[...snip...]

> diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c
> index 4e84e2f9987..80637503310 100644
> --- a/gcc/cp/name-lookup.c
> +++ b/gcc/cp/name-lookup.c

[...]

> +	  tree existing
> +	    = lookup_attribute ("diagnose_as", DECL_ATTRIBUTES (ns));
> +	  if (existing
> +		&& !cp_tree_equal(TREE_VALUE (args),
> +				  TREE_VALUE (TREE_VALUE (existing))))
> +	    {

Please add an auto_diagnostic_group here so that the "inform" is
associated with the "error".

> +	      error ("the namespace %qE already uses a different diagnose_as "
> +		     "attribute value", ns);

diagnose_as should be in quotes here (%< and %>).


> +	      inform (DECL_SOURCE_LOCATION (ns), "previous declaration here");
> +	      continue;
> +	    }
> +	  DECL_ATTRIBUTES (ns) = tree_cons (name, args,
> +					    DECL_ATTRIBUTES (ns));
> +	}
>        else
>  	{
>  	  warning (OPT_Wattributes, "%qD attribute directive ignored",
> diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c
> index a8bfd5fc053..f7b93dc89d7 100644
> --- a/gcc/cp/tree.c
> +++ b/gcc/cp/tree.c

[...snip...]

> +      else if (DECL_LANGUAGE (*node) == lang_c)
> +	{
> +	  error ("%qE attribute applied to extern \"C\" declaration %qD",

Please quote extern "C":
                                           %<extern \"C\"%>

> +		 name, *node);

[...snip...]

Thanks again for the patch; hope this is constructive
Dave