From: David Malcolm <dmalcolm@redhat.com>
To: Patrick Palka <ppalka@redhat.com>, Jason Merrill <jason@redhat.com>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] c++: fix source printing for "required from here" message
Date: Thu, 25 Apr 2024 14:48:44 -0400 [thread overview]
Message-ID: <4a899c6796eec17186918f9ffb116b1ed642caa4.camel@redhat.com> (raw)
In-Reply-To: <5930feb9-9460-457f-70c4-09c2c662d79e@idea>
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
> >
next prev parent reply other threads:[~2024-04-25 18:48 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-24 20:22 Patrick Palka
2024-04-24 20:54 ` Jason Merrill
2024-04-24 21:05 ` Patrick Palka
2024-04-25 18:48 ` David Malcolm [this message]
2024-04-26 11:52 ` Patrick Palka
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4a899c6796eec17186918f9ffb116b1ed642caa4.camel@redhat.com \
--to=dmalcolm@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jason@redhat.com \
--cc=ppalka@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).