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 [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 3BEFD395C01D for ; Tue, 4 May 2021 13:34:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 3BEFD395C01D Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-371-IAWxd8J3OeuAzpRzlgrxtw-1; Tue, 04 May 2021 09:34:16 -0400 X-MC-Unique: IAWxd8J3OeuAzpRzlgrxtw-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id CB6AA107ACCA; Tue, 4 May 2021 13:34:14 +0000 (UTC) Received: from t14s.localdomain (ovpn-112-65.phx2.redhat.com [10.3.112.65]) by smtp.corp.redhat.com (Postfix) with ESMTP id 39DCE60D52; Tue, 4 May 2021 13:34:13 +0000 (UTC) Message-ID: <72d4620b42f3bdbb3eab94a46ccc0434ed160fe0.camel@redhat.com> Subject: Re: [PATCH] Add gnu::diagnose_as attribute From: David Malcolm To: Matthias Kretz , gcc-patches@gcc.gnu.org Cc: libstdc++@gcc.gnu.org Date: Tue, 04 May 2021 09:34:13 -0400 In-Reply-To: <91863212.B8guWdUDZo@excalibur> References: <14205410.xuKvIAzr1H@excalibur> <20210427094648.GL3008@redhat.com> <91863212.B8guWdUDZo@excalibur> User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-13.6 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=unavailable autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 04 May 2021 13:34:19 -0000 On Tue, 2021-05-04 at 13:13 +0200, Matthias Kretz wrote: > From: Matthias Kretz > > This attribute overrides the diagnostics output string for the entity > it > appertains to. The motivation is to improve QoI for library TS > implementations, where diagnostics have a very bad signal-to-noise > ratio > due to the long namespaces involved. > > On Tuesday, 27 April 2021 11:46:48 CEST Jonathan Wakely wrote: > > I think it's a great idea and would like to use it for all the TS > > implementations where there is some inline namespace that the user > > doesn't care about. std::experimental::fundamentals_v1:: would be > > much > > better as just std::experimental::, or something like std::[LFTS]::. > > With the attribute, it is possible to solve PR89370 and make > std::__cxx11::basic_string<_CharT, _Traits, _Alloc> appear as > std::string in diagnostic output without extra hacks to recognize the > type. Thanks for the patch, it looks very promising. The C++ frontend maintainers will need to review the C++ frontend parts in detail, so I'll defer to them for the bulk of the review. Various thoughts: The patch has no testcases; it should probably add test coverage for: - the various places and ways in which diagnose_as can affect the output, - disabling it with the option - the various ways in which the user can get diagnose_as wrong - etc Does the patch affect the output of labels when underlining ranges of source code in diagnostics? Does the patch interact correctly with the %H and %I codes that try to show the differences between two template types? I have some minor nits from a diagnostics point of view: [...snip...] > diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c > index 4e84e2f9987..80637503310 100644 > --- a/gcc/cp/name-lookup.c > +++ b/gcc/cp/name-lookup.c [...] > + tree existing > + = lookup_attribute ("diagnose_as", DECL_ATTRIBUTES (ns)); > + if (existing > + && !cp_tree_equal(TREE_VALUE (args), > + TREE_VALUE (TREE_VALUE (existing)))) > + { Please add an auto_diagnostic_group here so that the "inform" is associated with the "error". > + error ("the namespace %qE already uses a different diagnose_as " > + "attribute value", ns); diagnose_as should be in quotes here (%< and %>). > + inform (DECL_SOURCE_LOCATION (ns), "previous declaration here"); > + continue; > + } > + DECL_ATTRIBUTES (ns) = tree_cons (name, args, > + DECL_ATTRIBUTES (ns)); > + } > else > { > warning (OPT_Wattributes, "%qD attribute directive ignored", > diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c > index a8bfd5fc053..f7b93dc89d7 100644 > --- a/gcc/cp/tree.c > +++ b/gcc/cp/tree.c [...snip...] > + else if (DECL_LANGUAGE (*node) == lang_c) > + { > + error ("%qE attribute applied to extern \"C\" declaration %qD", Please quote extern "C": % > + name, *node); [...snip...] Thanks again for the patch; hope this is constructive Dave