On Tue, 2021-05-04 at 13:13 +0200, Matthias Kretz wrote:
> From: Matthias Kretz <[email protected]>
>
> 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
