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 ESMTP id 089AF385780B for ; Thu, 27 May 2021 21:15:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 089AF385780B Received: from mail-qk1-f198.google.com (mail-qk1-f198.google.com [209.85.222.198]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-586-gGIvh0aEPqaH-tMesvT8yA-1; Thu, 27 May 2021 17:15:49 -0400 X-MC-Unique: gGIvh0aEPqaH-tMesvT8yA-1 Received: by mail-qk1-f198.google.com with SMTP id s123-20020a3777810000b02902e9adec2313so1450411qkc.4 for ; Thu, 27 May 2021 14:15:49 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=CeYOsUEF63lxDMUujfqb7IySsUF3+1cos2/FmFvYStM=; b=IEtYSKfuMd9BxL+f1q/glgcJ+QiD1p/QiauzeNW7Vs2y9LcV4zn57MBm81YgNE/sfB 8Kuifv5ZBAnaqlGU6StX1iI46NwmbB1Hir1/OssTZJAky/jRG9Eow2YiaL23QRuK6wbe EpG3kSXxXjQLWESPaI8wlmxB2d3+ykTcClLfhmgXBCKIM0jPC9/FlVVbHMGgG+NT/jhL j/QQN9g1Ib66kbL9699W3RW0jCs0WQR87mzNRQv3CEVlzWLurx1zs1MNq5new7/+QEqW pJjKJcu2U5wXFV0vmd5qyuiI6K2heJuoS9yvM2aprY34ny2ROGEFK9W5Fa8OYryQGtFQ UuBw== X-Gm-Message-State: AOAM531OI95BVNe6RRbOWxgoFI7eN8qv/OGV5B/bimEiKU24NwUSgJGl HntE74a8WSMUpZPLPY+JOHI2OWkvI6hyK12kus09sgZfuP4SSDFlOpNV4ePJJR/WHUb3HXvDoT0 9lrlVbgVZWFFCIuAbVQ== X-Received: by 2002:ac8:6051:: with SMTP id k17mr539194qtm.23.1622150148920; Thu, 27 May 2021 14:15:48 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwLVxAYtldchw8ARC50u3feiwj/Fg78k/qPSKs2DMJrOrpjXpE+rcu94DKBnit0CA7yxBrBqA== X-Received: by 2002:ac8:6051:: with SMTP id k17mr539178qtm.23.1622150148646; Thu, 27 May 2021 14:15:48 -0700 (PDT) Received: from [192.168.1.148] ([130.44.159.43]) by smtp.gmail.com with ESMTPSA id i11sm2089156qtv.8.2021.05.27.14.15.47 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 27 May 2021 14:15:47 -0700 (PDT) Subject: Re: [PATCH] Add gnu::diagnose_as attribute To: Matthias Kretz , gcc-patches@gcc.gnu.org Cc: libstdc++@gcc.gnu.org References: <14205410.xuKvIAzr1H@excalibur> <91863212.B8guWdUDZo@excalibur> <05673834-4912-e418-43cc-2dfdd45aabdf@redhat.com> <3483026.uDumqb6SjM@excalibur> From: Jason Merrill Message-ID: <512c40d3-b85d-f08e-f4b5-b9e40b2a4a41@redhat.com> Date: Thu, 27 May 2021 17:15:46 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 MIME-Version: 1.0 In-Reply-To: <3483026.uDumqb6SjM@excalibur> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-7.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham 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: Thu, 27 May 2021 21:15:56 -0000 On 5/27/21 2:54 PM, Matthias Kretz wrote: > On Thursday, 27 May 2021 19:39:48 CEST Jason Merrill wrote: >> On 5/4/21 7:13 AM, 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]::. >> >> Hmm, how much of the benefit could we get from a flag (probably on by >> default) to skip inline namespaces in diagnostics? > > I'd say about 20% for the TS's. Even std::experimental::simd (i.e. without the > '::parallelism_v2' part) is still rather noisy. I want stdₓ::simd, std-x::simd > or std::[PTS2]::simd or whatever shortest shorthand Jonathan will allow. ;) > > For PR89370, the benefit would be ~2%: > > 'template std::__cxx11::basic_string<_CharT, _Traits, > _Alloc>::_If_sv<_Tp, std::__cxx11::basic_string<_CharT, _Traits, _Alloc>&> > std::__cxx11::basic_string<_CharT, _Traits, > _Alloc>::insert(std::__cxx11::basic_string<_CharT, _Traits, > _Alloc>::size_type, const _Tp&, std::__cxx11::basic_string<_CharT, _Traits, > _Alloc>::size_type, std::__cxx11::basic_string<_CharT, _Traits, > _Alloc>::size_type) [with _Tp = _Tp; _CharT = char; _Traits = > std::char_traits; _Alloc = std::allocator]' > > would then only turn into: > > 'template std::basic_string<_CharT, _Traits, > _Alloc>::_If_sv<_Tp, std::basic_string<_CharT, _Traits, _Alloc>&> > std::basic_string<_CharT, _Traits, > _Alloc>::insert(std::basic_string<_CharT, _Traits, > _Alloc>::size_type, const _Tp&, std::basic_string<_CharT, _Traits, > _Alloc>::size_type, std::basic_string<_CharT, _Traits, > _Alloc>::size_type) [with _Tp = _Tp; _CharT = char; _Traits = > std::char_traits; _Alloc = std::allocator]' > > instead of: > > 'template std::string::_If_sv<_Tp, std::string&> > std::string::insert<_Tp>(std::string::size_type, const _Tp&, > std::string::size_type, std::string::size_type)' > > > Also hiding all inline namespace by default might make some error messages > harder to understand: > > namespace Vir { > inline namespace foo { > struct A {}; > } > struct A {}; > } > using Vir::A; > > > :7:12: error: reference to 'A' is ambiguous > :3:12: note: candidates are: 'struct Vir::A' > :5:10: note: 'struct Vir::A' That doesn't seem so bad. >>> 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. >> >> That sounds wrong to me; std::string is the instantiation, not >> the template. Your patch doesn't make it possible to apply this >> attribute to class template instantiations, does it? > > Yes, it does. > > Initially, when I tried to improve the TS experience, it didn't. When Jonathan > showed PR89370 to me I tried to make [[gnu::diagnose_as]] more generic & > useful. Since there's no obvious syntax to apply an attribute to a template > instantiation, I had to be creative. This is from my pending std::string > patch: > > --- a/libstdc++-v3/include/bits/c++config > +++ b/libstdc++-v3/include/bits/c++config > @@ -299,7 +299,8 @@ namespace std > #if _GLIBCXX_USE_CXX11_ABI > namespace std > { > - inline namespace __cxx11 __attribute__((__abi_tag__ ("cxx11"))) { } > + inline namespace __cxx11 > + __attribute__((__abi_tag__ ("cxx11"), __diagnose_as__("std"))) { } This seems to have the same benefits and drawbacks of my inline namespace suggestion. And it seems like applying the attribute to a namespace means that enclosing namespaces are not printed, unlike the handling for types. > } > namespace __gnu_cxx > { > --- a/libstdc++-v3/include/bits/stringfwd.h > +++ b/libstdc++-v3/include/bits/stringfwd.h > @@ -76,24 +76,24 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > _GLIBCXX_END_NAMESPACE_CXX11 > > /// A string of @c char > - typedef basic_string string; > + typedef basic_string string [[__gnu__::__diagnose_as__("string")]]; Here it seems like you want to say "use this typedef as the true name of the type". Is it useful to have to repeat the name? Allowing people to use names that don't correspond to actual declarations seems unnecessary. > #ifdef _GLIBCXX_USE_WCHAR_T > /// A string of @c wchar_t > - typedef basic_string wstring; > + typedef basic_string wstring > [[__gnu__::__diagnose_as__("wstring")]]; > #endif > [...] > > The part of my frontend patch that makes this work is in > handle_diagnose_as_attribute: > > + if (TREE_CODE (*node) == TYPE_DECL) > + { > + // Apply the attribute to the type alias itself. > + decl = *node; > + tree type = TREE_TYPE (*node); > + if (CLASS_TYPE_P (type) && CLASSTYPE_TEMPLATE_INSTANTIATION (type)) > + { > + if (COMPLETE_OR_OPEN_TYPE_P (type)) > + warning (OPT_Wattributes, > + "ignoring %qE attribute applied to template %qT after " > + "instantiation", name, type); > + else > + { > + type = strip_typedefs(type, nullptr, 0); > + // And apply the attribute to the specialization on the RHS. > + tree attributes = tree_cons (name, args, NULL_TREE); > + decl_attributes (&type, attributes, > + ATTR_FLAG_TYPE_IN_PLACE, NULL_TREE); > + } > + } > + } Ah, clever. Jason