From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id BA5203858C50 for ; Fri, 26 Apr 2024 11:53:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org BA5203858C50 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org BA5203858C50 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1714132387; cv=none; b=mWenfGpbcn/ZbwDgf/g2Z46tGHv7q/W47nG/CVBBjtuIHO4m7ZPDEgE9gTGg3L/pvFrsb7txEVAfyJNGekqJu7BxytavVvuPQU0dlmNjW1kpACdljO9Z11GA1D0YnbzP4s4UeZbU+JT+J4J3gAC79FrMwsdSjLdcsWQmmAqYT6M= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1714132387; c=relaxed/simple; bh=tOL684pFJvrLCyWcYfblWXSDDAX6eJUJ4QFtAmsJQNI=; h=DKIM-Signature:From:Date:To:Subject:Message-ID:MIME-Version; b=a8zfx6vSGKTcbbX9J03PyFyJEH0Gt8iObUJdX76dpl0hbvHNzzOl134nq1KgNSVNFdTloZP0unT7K4RtnnA7aFZOjl56xokJWU46DAVvC8C9/BTYWg5l1dc8xUb6AJndC0lAgytmVHlb6oIbRjomc5UWSCOqWI3AcCdE/cyjB3A= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1714132385; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=5H1hmAv/S8oayPgMtiOfIEOfOqf34HEFgZX6hwxZNDQ=; b=X5bIFuGccE5wM2IOxFxy5OXBnDuwGm4bUYPg8vBmiBIh6lPIYx8XVPB9WHfX3AXMKWQzFx QH6Jd8gEA1J4NscT3aTBHTRpHaJdEiEnWCPAHVj2t4rdXDACqpaf3CW0mw3hVOTmt+sHj1 nMu6pAOjf/zchoIcVvl3w5QtdFfxj3g= Received: from mail-qt1-f199.google.com (mail-qt1-f199.google.com [209.85.160.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-128-GNOfsPNkN1-p5gRgLafexA-1; Fri, 26 Apr 2024 07:53:04 -0400 X-MC-Unique: GNOfsPNkN1-p5gRgLafexA-1 Received: by mail-qt1-f199.google.com with SMTP id d75a77b69052e-439d6a512e9so28309391cf.1 for ; Fri, 26 Apr 2024 04:53:04 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714132381; x=1714737181; h=mime-version:references:message-id:in-reply-to:subject:cc:to:date :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=FyAy1Mdh6zBTU7aGuPLzcoHVEYc7Av4zLsfv+54CLrE=; b=fb9Ml3fQVaRKoYGm7VeruP9uwsvgd7pyuTix6JaPqn4Imkh3kYPAn65IskiWCssIX7 1azgBxXuicG4NL+LwCWDFg3+fTkIbh+CAgLRDJAsdJhush3dE6yol3AOIogisr+dZ4Ti UP14Px1YJnN8QOZEoKoLSm7x18SeUAroA/C/9hVPCUo55tbbOWXB/9JsOirq9B9EnOqB ysHUi9DwTm3q/9diImdnvpSjqCSVjrgfee2wZTV79js/iJj2WLDagPdWXQ1cY2Hhd5wI jTj755d2pGhAhSx/Cc3M2/8jPAwAhDKs9/6HKkW4ZMZ789rmbRt5ekO7mVeCp+Zi3FXK zatA== X-Forwarded-Encrypted: i=1; AJvYcCXvn9APMpO2Fz7HPTeuxDs9iCTun98RXlqS0nhrG44CB6pcDngI4ivvtlWRdOZQgjEdz09sQo0/6RCVqAwc7R5u3e2X0pqD1w== X-Gm-Message-State: AOJu0YzawUBoDsiKF9iOxfBdyNALcLs0C1tv14HjWPY0AIi3TeYGKQRr 7RO428+1RHNlzbT98/+89OM2zYcqcytHnhlufX4T5FwHcEH0kTw8Il1ZQxeYYXiXIUxIg2CKq6a e170im8EJfLPYgUkEVWRhdpNm5Q5N6gHV2m+SMBz3WBOobOwHyMAD53o= X-Received: by 2002:a05:622a:178a:b0:439:8cc6:d696 with SMTP id s10-20020a05622a178a00b004398cc6d696mr2771039qtk.33.1714132380812; Fri, 26 Apr 2024 04:53:00 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHqK7qBKhM2ejKNnQwcjYs0ivpZBsqRuXzube2Yd5KM9qL96ZBAp96X5VDjTPIN6AFeyVBZ1g== X-Received: by 2002:a05:622a:178a:b0:439:8cc6:d696 with SMTP id s10-20020a05622a178a00b004398cc6d696mr2771020qtk.33.1714132380300; Fri, 26 Apr 2024 04:53:00 -0700 (PDT) Received: from [192.168.1.130] (ool-457670bb.dyn.optonline.net. [69.118.112.187]) by smtp.gmail.com with ESMTPSA id o15-20020ac8428f000000b0042f04e421d2sm7800567qtl.24.2024.04.26.04.52.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 26 Apr 2024 04:52:59 -0700 (PDT) From: Patrick Palka X-Google-Original-From: Patrick Palka Date: Fri, 26 Apr 2024 07:52:59 -0400 (EDT) To: David Malcolm cc: Patrick Palka , Jason Merrill , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] c++: fix source printing for "required from here" message In-Reply-To: <4a899c6796eec17186918f9ffb116b1ed642caa4.camel@redhat.com> Message-ID: References: <20240424202206.173103-1-ppalka@redhat.com> <8bcaf757-d1fe-4ea5-8128-77b6fbf465e3@redhat.com> <5930feb9-9460-457f-70c4-09c2c662d79e@idea> <4a899c6796eec17186918f9ffb116b1ed642caa4.camel@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/mixed; boundary="8323329-525192606-1714132379=:3760645" X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00,BODY_8BITS,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323329-525192606-1714132379=:3760645 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT 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 > > > > (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 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::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 (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 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::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 > > > > > --8323329-525192606-1714132379=:3760645--