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

[Jason Merrill <jason@redhat.com>]

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.

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