Re: [PATCH] RFC: elide repeated source locations (PR other/84889)

[Eric Gallager <egall at gwmail dot gwu dot edu>]

On 11/15/18, Martin Sebor <msebor@gmail.com> wrote:
> On 11/14/2018 02:12 PM, David Malcolm wrote:
>> On Mon, 2018-11-12 at 13:37 -0700, Martin Sebor wrote:
>>> On 11/11/2018 07:43 PM, David Malcolm wrote:
>>>> We often emit more than one diagnostic at the same source location.
>>>> For example, the C++ frontend can emit many diagnostics at
>>>> the same source location when suggesting overload candidates.
>>>>
>>>> For example:
>>>>
>>>> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C: In
>>>> function 'int test_3(s, t)':
>>>> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18:
>>>> error: no match for 'operator&&' (operand types are 's' and 't')
>>>>    38 |   return param_s && param_t;
>>>>       |          ~~~~~~~~^~~~~~~~~~
>>>> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18:
>>>> note: candidate: 'operator&&(bool, bool)' <built-in>
>>>> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18:
>>>> note:   no known conversion for argument 2 from 't' to 'bool'
>>>>
>>>> This is overly verbose.  Note how the same location has been
>>>> printed
>>>> three times, obscuring the pertinent messages.
>>>>
>>>> This patch add a new "elide" value to -fdiagnostics-show-location=
>>>> and makes it the default (previously it was "once").  With elision
>>>> the above is printed as:
>>>>
>>>> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C: In
>>>> function 'int test_3(s, t)':
>>>> ../../src/gcc/testsuite/g++.dg/diagnostic/bad-binary-ops.C:38:18:
>>>> error: no match for 'operator&&' (operand types are 's' and 't')
>>>>    38 |   return param_s && param_t;
>>>>       |          ~~~~~~~~^~~~~~~~~~
>>>>       = note: candidate: 'operator&&(bool, bool)' <built-in>
>>>>       = note:   no known conversion for argument 2 from 't' to
>>>> 'bool'
>>>>
>>>> where the followup notes are printed with a '=' lined up with
>>>> the source code margin.
>>>>
>>>> Thoughts?
>>>
>>> I agree the long pathname in the notes is at first glance redundant
>>> but I'm not sure about using '=' as a shorthand for it.

I like the -fdiagnostics-show-location=elide option but I also agree
about the '=' looking weird. However, I don't think the '=' is that
big a problem that needs to provoke huge rethinking, just some minor
bikeshedding about which symbol to use instead. How about '+'?

>>> I have
>>> written many scripts to parse GCC output to extract all diagnostics
>>> (including notes) and publish those on a Web page somewhere, as I'm
>>> sure must have others.  All those scripts would stop working with
>>> this change and require changes to the build system to work again.
>>> Making those changes can be a substantial undertaking in some
>>> organizations.
>>>
>>> Have you considered printing just the file name instead?  Or any
>>> other alternatives?
>>
>> "-fdiagnostics-show-location=once" will restore the old behavior.
>> Alternatively, if you want to parse GCC output, I'm adding a JSON
>> output format; see:
>>   https://gcc.gnu.org/ml/gcc-patches/2018-11/msg01038.html
>> (I'm testing an updated version of that patch)
>
> I understand the change can be disabled by an option.  I also
> like making the repetitive pathnames shorter, but the concern
> I have is that the change to use the '=' notation by default,
> besides being rather unusual and not necessarily intuitive, will
> break scripts that search for the pattern:
>
>    "[^:]*:[1-9][0-9]*:[1-9][0-9]*: (error|note|warning): "
>
> and adding a new option to a large build system can be quite
> difficult.  I suspect it would also make the notes difficult
> or even impossible to associate with the corresponding errors
> or warnings in parallel builds (where they may be interleaved
> with diagnostics from different compilations).
>
> I think it's possible to make the output shorter/less repetitive
> without these side-effects by keeping the current format and
> instead abbreviating the pathname, e.g. by printing just the file
> name (or ".../filename.c:" with the ellipsis standing in for the
> long pathname, though the ellipsis would require adjusting
> the scripts that sort diagnostics by the pathname).
>
> Martin
>
> PS As an aside, if we wanted to shorten all pathnames this same
> idea could be extended to errors and warnings as well by printing
> the pathname in the first one and just the filename in subsequent
> ones in the same file.
>