Re: [PATCH] Add gnu::diagnose_as attribute

[Jason Merrill <jason@redhat.com>]

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 <kretz@kde.org>
>>>
>>> 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<class _Tp> 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<char>; _Alloc = std::allocator<char>]'
> 
> would then only turn into:
> 
> 'template<class _Tp> 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<char>; _Alloc = std::allocator<char>]'
> 
> instead of:
> 
> 'template<class _Tp> 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;
> 
> 
> <source>:7:12: error: reference to 'A' is ambiguous
> <source>:3:12: note: candidates are: 'struct Vir::A'
> <source>: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 <char> 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<char>    string;
> +  typedef basic_string<char> 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<wchar_t> wstring;
> +  typedef basic_string<wchar_t> 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