Re: [PATCH] c++: fix source printing for "required from here" message

[Patrick Palka <ppalka@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5328 bytes --]

On Wed, 24 Apr 2024, Jason Merrill wrote:

> On 4/24/24 13:22, Patrick Palka wrote:
> > Tested on x86_64-pc-linux-gnu, full bootstrap+regtest in progress,
> > does this look OK if successful?
> > 
> > -- >8 --
> > 
> > It seems the diagnostic machinery's source line printing respects
> > the pretty printer prefix, but this is undesirable for the call to
> > diagnostic_show_locus in print_instantiation_partial_context_line
> > added in r14-4388-g1c45319b66edc9 since the prefix may have been
> > set when issuing an earlier, unrelated diagnostic and we just want
> > to print an unprefixed source line.
> > 
> > This patch naively fixes this by clearing the prefix before calling
> > diagnostic_show_locus.
> > 
> > Before this patch, for error60a.C below we'd print
> > 
> > gcc/testsuite/g++.dg/template/error60a.C: In function ‘void usage()’:
> > gcc/testsuite/g++.dg/template/error60a.C:24:3: error: ‘unrelated_error’ was
> > not declared in this scope
> >     24 |   unrelated_error; // { dg-error "not declared" }
> >        |   ^~~~~~~~~~~~~~~
> > gcc/testsuite/g++.dg/template/error60a.C: In instantiation of ‘void
> > test(Foo) [with Foo = int]’:
> > gcc/testsuite/g++.dg/template/error60a.C:25:13:   required from here
> > gcc/testsuite/g++.dg/template/error60a.C:24:3: error:    25 |   test<int>
> > (42); // { dg-message " required from here" }
> > gcc/testsuite/g++.dg/template/error60a.C:24:3: error:       |
> > ~~~~~~~~~~^~~~
> > gcc/testsuite/g++.dg/template/error60a.C:19:24: error: invalid conversion
> > from ‘int’ to ‘int*’ [-fpermissive]
> >     19 |   my_pointer<Foo> ptr (val); // { dg-error "invalid conversion from
> > 'int' to 'int\\*'" }
> >        |                        ^~~
> >        |                        |
> >        |                        int
> > gcc/testsuite/g++.dg/template/error60a.C:9:20: note:   initializing argument
> > 1 of ‘my_pointer<Foo>::my_pointer(Foo*) [with Foo = int]’
> >      9 |   my_pointer (Foo *ptr) // { dg-message " initializing argument 1"
> > }
> >        |               ~~~~~^~~
> > 
> > and afterward we print
> > 
> > gcc/testsuite/g++.dg/template/error60a.C: In function ‘void usage()’:
> > gcc/testsuite/g++.dg/template/error60a.C:24:3: error: ‘unrelated_error’ was
> > not declared in this scope
> >     24 |   unrelated_error; // { dg-error "not declared" }
> >        |   ^~~~~~~~~~~~~~~
> > gcc/testsuite/g++.dg/template/error60a.C: In instantiation of ‘void
> > test(Foo) [with Foo = int]’:
> > gcc/testsuite/g++.dg/template/error60a.C:25:13:   required from here
> >     25 |   test<int> (42); // { dg-message " required from here" }
> >        |   ~~~~~~~~~~^~~~
> > gcc/testsuite/g++.dg/template/error60a.C:19:24: error: invalid conversion
> > from ‘int’ to ‘int*’ [-fpermissive]
> >     19 |   my_pointer<Foo> ptr (val); // { dg-error "invalid conversion from
> > 'int' to 'int\\*'" }
> >        |                        ^~~
> >        |                        |
> >        |                        int
> > gcc/testsuite/g++.dg/template/error60a.C:9:20: note:   initializing argument
> > 1 of ‘my_pointer<Foo>::my_pointer(Foo*) [with Foo = int]’
> >      9 |   my_pointer (Foo *ptr) // { dg-message " initializing argument 1"
> > }
> >        |               ~~~~~^~~
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* error.cc (print_instantiation_partial_context_line): Clear
> > 	context->printer->prefix around the call to diagnostic_show_locus.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/concepts/diagnostic2.C: Expect source line printed
> > 	for the required from here message.
> > 	* g++.dg/template/error60a.C: New test.
> > ---
> >   gcc/cp/error.cc                             |  2 +
> >   gcc/testsuite/g++.dg/concepts/diagnostic2.C |  6 ++-
> >   gcc/testsuite/g++.dg/template/error60a.C    | 46 +++++++++++++++++++++
> >   3 files changed, 53 insertions(+), 1 deletion(-)
> >   create mode 100644 gcc/testsuite/g++.dg/template/error60a.C
> > 
> > diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc
> > index 7074845154e..a7067d4d2ed 100644
> > --- a/gcc/cp/error.cc
> > +++ b/gcc/cp/error.cc
> > @@ -3793,7 +3793,9 @@ print_instantiation_partial_context_line
> > (diagnostic_context *context,
> >   		   : _("required from here\n"));
> >       }
> >     gcc_rich_location rich_loc (loc);
> > +  char *saved_prefix = pp_take_prefix (context->printer);
> >     diagnostic_show_locus (context, &rich_loc, DK_NOTE);
> > +  context->printer->prefix = saved_prefix;
> 
> I would follow the pattern of c_diagnostic_finalizer here, i.e. using
> pp_set_prefix for the restore.

FWIW that's what I originally went with, but I don't really understand
the other things pp_set_prefix does besides setting the prefix and
then I noticed cp_print_error_function restores ->prefix directly so
I ended up doing that instead.

> 
> Though I think the pp_set_prefix to NULL in c_diagnostic_finalizer is
> redundant and should have been removed in r9-2240-g653fee1961981f when the
> previous line changed from _set_ to _take_.  If it isn't redundant, then it
> should be, i.e. pp_take_prefix should call it instead of directly setting
> NULL.
> 
> Some _take_ callers do set(NULL) and others don't; they should definitely be
> consistent one way or the other.
> 
> David, what do you think?
> 
> Jason
> 
>