Re: [PATCH] Add gnu::diagnose_as attribute

[David Malcolm <dmalcolm@redhat.com>]

On Tue, 2021-05-04 at 16:23 +0200, Matthias Kretz wrote:
> On Tuesday, 4 May 2021 15:34:13 CEST David Malcolm wrote:
> > On Tue, 2021-05-04 at 13:13 +0200, Matthias Kretz wrote:
> > > 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.
> > > [...]
> > 
> > Thanks for the patch, it looks very promising.
> 
> Thanks. I'm new to modifying the compiler like this, so please be
> extra 
> careful with my patch. I believe I understand most of what I did, but
> I might 
> have misunderstood. :)

That's tends to be how I feel when working on the C++ FE :)

> 
> > 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
> 
> Right. If you know of an existing similar testcase, that'd help me a
> lot to 
> get started.

FWIW I implemented the %H and %I codes in
f012c8ef4b35dcee9b5a3807868d050812d5b3b9 and that commit adds various
DejaGnu testcases that exercise C++ diagnostics with and without
various options, verifying the precise wording of errors.  So that
might be a good place to look.


> 
> > Does the patch affect the output of labels when underlining ranges of
> > source code in diagnostics?
> 
> AFAIU (and tested), it doesn't affect source code output. So, no?

Sorry, I was unclear.  I was referring to this kind of thing:

$ cat t.C
#include <string>

std::string test (std::string s)
{
    return &s;
}

$ g++ t.C
t.C: In function ‘std::string test(std::string)’:
t.C:5:12: error: could not convert ‘& s’ from ‘std::string*’ {aka
‘std::__cxx11::basic_string<char>*’} to ‘std::string’ {aka
‘std::__cxx11::basic_string<char>’}
    5 |     return &s;
      |            ^~
      |            |
      |            std::string* {aka std::__cxx11::basic_string<char>*}

i.e. the final line in the output above, where the underlined range of
source code in line 5 is labelled, showing the type of the expression.

Hopefully it ought to automatically just drop out of the work you've
already done.

FWIW an example of implementation this can be seen in
a14feb3c783fba6af8d66b8138214a3a313be5c5 (which added labels for type
errors in C++ binary operators).


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

I don't know. I'll try to find out. If you have a good idea (or
pointer) for a 
testcase, let me know.

See above.


Dave