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
>
>
