* [PATCH] c++: fix source printing for "required from here" message
@ 2024-04-24 20:22 Patrick Palka
2024-04-24 20:54 ` Jason Merrill
0 siblings, 1 reply; 5+ messages in thread
From: Patrick Palka @ 2024-04-24 20:22 UTC (permalink / raw)
To: gcc-patches; +Cc: jason, dmalcolm, Patrick Palka
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;
}
/* Same as print_instantiation_full_context but less verbose. */
diff --git a/gcc/testsuite/g++.dg/concepts/diagnostic2.C b/gcc/testsuite/g++.dg/concepts/diagnostic2.C
index 6550ed6b3bd..d6f5872de2c 100644
--- a/gcc/testsuite/g++.dg/concepts/diagnostic2.C
+++ b/gcc/testsuite/g++.dg/concepts/diagnostic2.C
@@ -23,7 +23,11 @@ void
baz()
{
bar<int>(); // { dg-error "no match" }
-/* { dg-begin-multiline-output "" }
+/* { dg-begin-multiline-output "for no match error" }
+ bar<int>();
+ ~~~~~~~~^~
+ { dg-end-multiline-output "" } */
+/* { dg-begin-multiline-output "for required from here message" }
bar<int>();
~~~~~~~~^~
{ dg-end-multiline-output "" } */
diff --git a/gcc/testsuite/g++.dg/template/error60a.C b/gcc/testsuite/g++.dg/template/error60a.C
new file mode 100644
index 00000000000..7c2f8188d16
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/error60a.C
@@ -0,0 +1,46 @@
+// Like error60.C but first issues an unrelated error that
+// causes the pretty printer prefix to get set, verifying we
+// still print the source line for the "required from here"
+// message correctly in that case.
+// { dg-options "-fdiagnostics-show-caret" }
+
+template <typename Foo>
+struct my_pointer
+{
+ my_pointer (Foo *ptr) // { dg-message " initializing argument 1" }
+ : m_ptr (ptr)
+ {}
+
+ Foo *m_ptr;
+};
+
+template <typename Foo>
+void test (Foo val)
+{
+ my_pointer<Foo> ptr (val); // { dg-error "invalid conversion from 'int' to 'int\\*'" }
+}
+
+void usage ()
+{
+ unrelated_error; // { dg-error "not declared" }
+ test<int> (42); // { dg-message " required from here" }
+ /* { dg-begin-multiline-output "" }
+ unrelated_error;
+ ^~~~~~~~~~~~~~~
+ { dg-end-multiline-output "" } */
+ /* { dg-begin-multiline-output "" }
+ test<int> (42);
+ ~~~~~~~~~~^~~~
+ { dg-end-multiline-output "" } */
+}
+
+ /* { dg-begin-multiline-output "" }
+ my_pointer (Foo *ptr)
+ ~~~~~^~~
+ { dg-end-multiline-output "" } */
+ /* { dg-begin-multiline-output "" }
+ my_pointer<Foo> ptr (val);
+ ^~~
+ |
+ int
+ { dg-end-multiline-output "" } */
--
2.45.0.rc1
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] c++: fix source printing for "required from here" message
2024-04-24 20:22 [PATCH] c++: fix source printing for "required from here" message Patrick Palka
@ 2024-04-24 20:54 ` Jason Merrill
2024-04-24 21:05 ` Patrick Palka
0 siblings, 1 reply; 5+ messages in thread
From: Jason Merrill @ 2024-04-24 20:54 UTC (permalink / raw)
To: Patrick Palka, gcc-patches; +Cc: dmalcolm
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] c++: fix source printing for "required from here" message
2024-04-24 20:54 ` Jason Merrill
@ 2024-04-24 21:05 ` Patrick Palka
2024-04-25 18:48 ` David Malcolm
0 siblings, 1 reply; 5+ messages in thread
From: Patrick Palka @ 2024-04-24 21:05 UTC (permalink / raw)
To: Jason Merrill; +Cc: Patrick Palka, gcc-patches, dmalcolm
[-- 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
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] c++: fix source printing for "required from here" message
2024-04-24 21:05 ` Patrick Palka
@ 2024-04-25 18:48 ` David Malcolm
2024-04-26 11:52 ` Patrick Palka
0 siblings, 1 reply; 5+ messages in thread
From: David Malcolm @ 2024-04-25 18:48 UTC (permalink / raw)
To: Patrick Palka, Jason Merrill; +Cc: gcc-patches
On Wed, 2024-04-24 at 17:05 -0400, Patrick Palka wrote:
> 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.
I have a slight preference for using pp_set_prefix for the restore, but
the patch as written is also OK; thanks.
I confess that I don't have a strong sense of how the prefix code is
meant to work.
It seems to be for use with this combination of options:
-fmessage-length=NON_ZERO -fdiagnostics-show-location=every-line
which triggers line-wrapping, adding the prefix at each line when line
wrapping occurs. This seems to have existed at least as far back as
856b62442f6fc5e4302ae9ee1ebce8a19bbd8681
re this "C++ err msgs" thread from 2000:
https://gcc.gnu.org/pipermail/libstdc++/2000-May/004650.html
https://gcc.gnu.org/pipermail/libstdc++/2000-May/004664.html
https://gcc.gnu.org/pipermail/gcc-patches/2000-May/031143.html
which made the prefixing optional on continuation lines.
I suspect that if anyone was using that combination of options, we've
probably broken it at some point with quoting of source code,
underlining, fix-it hints, etc etc
Dave
>
> >
> > 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
> >
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] c++: fix source printing for "required from here" message
2024-04-25 18:48 ` David Malcolm
@ 2024-04-26 11:52 ` Patrick Palka
0 siblings, 0 replies; 5+ messages in thread
From: Patrick Palka @ 2024-04-26 11:52 UTC (permalink / raw)
To: David Malcolm; +Cc: Patrick Palka, Jason Merrill, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 7749 bytes --]
On Thu, 25 Apr 2024, David Malcolm wrote:
> On Wed, 2024-04-24 at 17:05 -0400, Patrick Palka wrote:
> > 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.
>
> I have a slight preference for using pp_set_prefix for the restore, but
> the patch as written is also OK; thanks.
Thanks a lot, pushed as r15-4-g7d5479a2ecf630 which uses pp_set_prefix
for the restore as suggested.
>
> I confess that I don't have a strong sense of how the prefix code is
> meant to work.
>
> It seems to be for use with this combination of options:
> -fmessage-length=NON_ZERO -fdiagnostics-show-location=every-line
> which triggers line-wrapping, adding the prefix at each line when line
> wrapping occurs. This seems to have existed at least as far back as
> 856b62442f6fc5e4302ae9ee1ebce8a19bbd8681
> re this "C++ err msgs" thread from 2000:
>
> https://gcc.gnu.org/pipermail/libstdc++/2000-May/004650.html
> https://gcc.gnu.org/pipermail/libstdc++/2000-May/004664.html
> https://gcc.gnu.org/pipermail/gcc-patches/2000-May/031143.html
>
> which made the prefixing optional on continuation lines.
>
> I suspect that if anyone was using that combination of options, we've
> probably broken it at some point with quoting of source code,
> underlining, fix-it hints, etc etc
>
> Dave
>
>
>
>
> >
> > >
> > > 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
> > >
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-04-26 11:53 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-24 20:22 [PATCH] c++: fix source printing for "required from here" message Patrick Palka
2024-04-24 20:54 ` Jason Merrill
2024-04-24 21:05 ` Patrick Palka
2024-04-25 18:48 ` David Malcolm
2024-04-26 11:52 ` Patrick Palka
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).