Re: [PATCH] Add gnu::diagnose_as attribute
On 7/7/21 4:23 AM, Matthias Kretz wrote: On Tuesday, 22 June 2021 21:52:16 CEST Jason Merrill wrote: 2. About the namespace aliases: IIUC an attribute would currently be rejected because of the C++ grammar. Do you want to make it valid before WG21 officially decides how to proceed? And if you have a pointer for me where I'd have to adjust the grammar rules, that'd help. You will want to adjust cp_parser_namespace_alias_definition to handle attributes like cp_parser_namespace_definition. The latter currently accepts attributes both before and after the name, which seems like a good pattern to follow so it doesn't matter which WG21 chooses. Probably best to pedwarn about C++11 attributes in both locations for now, not just after. This introduces an ambiguity in cp_parser_declaration. The function has to decide whether to call cp_parser_namespace_definition or fall back to cp_parser_block_declaration (which calls cp_parser_namespace_alias_definition). But now the parser has to look ahead a lot farther: namespace foo [[whatever]] {} namespace bar [[whatever]] = foo; I.e. only at '{' vs. '=' can cp_parser_declaration decide to call cp_parser_namespace_definition. Consequently, should I really modify cp_parser_namespace_definition to handle namespace aliases? aliases can also appear at block scope, unlike namespace definitions, but you could factor out some of the alias handling to call from both places. Or can/should cp_parser_declaration look ahead behind the attribute(s)? How? cp_parser_skip_attributes_opt Jason
Re: [PATCH] Add gnu::diagnose_as attribute
On 7/5/21 10:18 AM, Matthias Kretz wrote: On Thursday, 1 July 2021 17:18:26 CEST Jason Merrill wrote: You probably want to adjust is_late_template_attribute to change that. Right, I hacked is_late_template_attribute but now I only see a TYPE_DECL passed to my attribute handler (!DECL_ALIAS_TEMPLATE_P). > I.e. I don't know how your previous comment is supposed to help me: On Tuesday, 22 June 2021 22:12:42 CEST Jason Merrill wrote: Yes. You can check that with get_underlying_template. FWIW, I don't feel qualified to implement the diagnose_as attribute on alias templates. The trees I've seen while testing the following test case don't make sense to me. :( // { dg-do compile { target c++11 } } // { dg-options "-fdiagnostics-use-aliases -fpretty-templates" } template class A0 {}; template using B0 [[gnu::diagnose_as]] = A0; // #1 template using C0 [[gnu::diagnose_as]] = A0; // #2 template class A1 {}; template class A1 {}; template using B1 [[gnu::diagnose_as]] = A1; // #3 void fn_1(int); int main () { fn_1 (A0 ()); // { dg-error "cannot convert 'B0' to 'int'" } fn_1 (A1 ()); // { dg-error "cannot convert 'A1' to 'int'" } fn_1 (A1 ()); // { dg-error "cannot convert 'B1' to 'int'" } } On #1 I see !COMPLETE_TYPE_P (TREE_TYPE (*node)) Yes; a use that matches the primary template but is not the primary template gets a separate dependent type. while on #3 TREE_TYPE (*node) is a complete type. Indeed, we don't do the same thing for partial specializations. Like I said, I don't get to see the TEMPLATE_DECL of either #1, #2, or #3, only a TYPE_DECL whose TREE_TYPE is A0. I thus have no idea how to reject #2. The difference between #1 and #2 is that same_type_p (#1, CLASSTYPE_PRIMARY_TEMPLATE_TYPE (TREE_TYPE (#1)) is true; see the use of that in resolve_typename_type. Jason
Re: [PATCH] Add gnu::diagnose_as attribute
On Tuesday, 22 June 2021 21:52:16 CEST Jason Merrill wrote: > > 2. About the namespace aliases: IIUC an attribute would currently be > > rejected because of the C++ grammar. Do you want to make it valid before > > WG21 officially decides how to proceed? And if you have a pointer for me > > where I'd have to adjust the grammar rules, that'd help. > > You will want to adjust cp_parser_namespace_alias_definition to handle > attributes like cp_parser_namespace_definition. The latter currently > accepts attributes both before and after the name, which seems like a > good pattern to follow so it doesn't matter which WG21 chooses. > Probably best to pedwarn about C++11 attributes in both locations for > now, not just after. This introduces an ambiguity in cp_parser_declaration. The function has to decide whether to call cp_parser_namespace_definition or fall back to cp_parser_block_declaration (which calls cp_parser_namespace_alias_definition). But now the parser has to look ahead a lot farther: namespace foo [[whatever]] {} namespace bar [[whatever]] = foo; I.e. only at '{' vs. '=' can cp_parser_declaration decide to call cp_parser_namespace_definition. Consequently, should I really modify cp_parser_namespace_definition to handle namespace aliases? Or can/should cp_parser_declaration look ahead behind the attribute(s)? How? With pedantic standard C++ it would be easy, since only these attribute placements are allowed: namespace [[whatever] foo {} namespace bar [[whatever]] = foo; -- ── Dr. Matthias Kretz https://mattkretz.github.io GSI Helmholtz Centre for Heavy Ion Research https://gsi.de std::experimental::simd https://github.com/VcDevel/std-simd ──
Re: [PATCH] Add gnu::diagnose_as attribute
On Thursday, 1 July 2021 17:18:26 CEST Jason Merrill wrote: > You probably want to adjust is_late_template_attribute to change that. Right, I hacked is_late_template_attribute but now I only see a TYPE_DECL passed to my attribute handler (!DECL_ALIAS_TEMPLATE_P). I.e. I don't know how your previous comment is supposed to help me: On Tuesday, 22 June 2021 22:12:42 CEST Jason Merrill wrote: > Yes. You can check that with get_underlying_template. FWIW, I don't feel qualified to implement the diagnose_as attribute on alias templates. The trees I've seen while testing the following test case don't make sense to me. :( // { dg-do compile { target c++11 } } // { dg-options "-fdiagnostics-use-aliases -fpretty-templates" } template class A0 {}; template using B0 [[gnu::diagnose_as]] = A0; // #1 template using C0 [[gnu::diagnose_as]] = A0; // #2 template class A1 {}; template class A1 {}; template using B1 [[gnu::diagnose_as]] = A1; // #3 void fn_1(int); int main () { fn_1 (A0 ()); // { dg-error "cannot convert 'B0' to 'int'" } fn_1 (A1 ()); // { dg-error "cannot convert 'A1' to 'int'" } fn_1 (A1 ()); // { dg-error "cannot convert 'B1' to 'int'" } } On #1 I see !COMPLETE_TYPE_P (TREE_TYPE (*node)) while on #3 TREE_TYPE (*node) is a complete type. Like I said, I don't get to see the TEMPLATE_DECL of either #1, #2, or #3, only a TYPE_DECL whose TREE_TYPE is A0. I thus have no idea how to reject #2. -- ── Dr. Matthias Kretz https://mattkretz.github.io GSI Helmholtz Centre for Heavy Ion Research https://gsi.de std::experimental::simd https://github.com/VcDevel/std-simd ──
Re: [PATCH] Add gnu::diagnose_as attribute
On 7/1/21 5:28 AM, Matthias Kretz wrote: On Tuesday, 22 June 2021 22:12:42 CEST Jason Merrill wrote: On 6/22/21 4:01 PM, Matthias Kretz wrote: On Tuesday, 22 June 2021 21:52:16 CEST Jason Merrill wrote: For alias templates, you probably want the attribute only on the templated class, not on the instantiations. Oh good point. My current patch does not allow the attribute on alias templates. Consider: template struct X {}; template using foo [[gnu::diagnose_as]] = X; I have no idea how this could work. I would have to set the attribute for an implicit partial specialization (not that I know of the existence of such a thing)? I.e. X would have to be diagnosed as foo, but X would have to be diagnosed as X, not foo. So if anything it should only support alias templates if they are strictly "renaming" the type. I.e. their template parameters must match up exactly. Can I constrain the attribute like this? Yes. You can check that with get_underlying_template. Or you could support the above by putting the attribute on the instantiation with the TEMPLATE_INFO for foo rather than a simple name. Question, given: template using foo = bar; The diagnose_as attribute handler isn't called until e.g. `foo` is instantiated. You probably want to adjust is_late_template_attribute to change that. Meaning that even after the declaration of the alias template `bar` will not be diagnosed as `foo`, which happens only after the first use of `foo`. I find that more confusing than helpful, even if the expectation would be that users only use the alias template. So do you still expect alias templates to support diagnose_as? And if yes, how could I handle the attribute so that the diagnose_as attribute is applied to `bar` on declaration of `foo`?
Re: [PATCH] Add gnu::diagnose_as attribute
On Tuesday, 22 June 2021 22:12:42 CEST Jason Merrill wrote: > On 6/22/21 4:01 PM, Matthias Kretz wrote: > > On Tuesday, 22 June 2021 21:52:16 CEST Jason Merrill wrote: > >> For alias templates, you probably want the attribute only on the > >> templated class, not on the instantiations. > > > > Oh good point. My current patch does not allow the attribute on alias > > templates. Consider: > > > > template > > > >struct X {}; > > > > template > > > >using foo [[gnu::diagnose_as]] = X; > > > > I have no idea how this could work. I would have to set the attribute for > > an implicit partial specialization (not that I know of the existence of > > such a thing)? I.e. X would have to be diagnosed as foo, > > but X would have to be diagnosed as X, not foo. > > > > So if anything it should only support alias templates if they are strictly > > "renaming" the type. I.e. their template parameters must match up exactly. > > Can I constrain the attribute like this? > > Yes. You can check that with get_underlying_template. > > Or you could support the above by putting the attribute on the > instantiation with the TEMPLATE_INFO for foo rather than a simple name. Question, given: template using foo = bar; The diagnose_as attribute handler isn't called until e.g. `foo` is instantiated. Meaning that even after the declaration of the alias template `bar` will not be diagnosed as `foo`, which happens only after the first use of `foo`. I find that more confusing than helpful, even if the expectation would be that users only use the alias template. So do you still expect alias templates to support diagnose_as? And if yes, how could I handle the attribute so that the diagnose_as attribute is applied to `bar` on declaration of `foo`? -- ── Dr. Matthias Kretz https://mattkretz.github.io GSI Helmholtz Centre for Heavy Ion Research https://gsi.de std::experimental::simd https://github.com/VcDevel/std-simd ──
Re: [PATCH] Add gnu::diagnose_as attribute
On 6/22/21 4:01 PM, Matthias Kretz wrote: On Tuesday, 22 June 2021 21:52:16 CEST Jason Merrill wrote: For alias templates, you probably want the attribute only on the templated class, not on the instantiations. Oh good point. My current patch does not allow the attribute on alias templates. Consider: template struct X {}; template using foo [[gnu::diagnose_as]] = X; I have no idea how this could work. I would have to set the attribute for an implicit partial specialization (not that I know of the existence of such a thing)? I.e. X would have to be diagnosed as foo, but X would have to be diagnosed as X, not foo. So if anything it should only support alias templates if they are strictly "renaming" the type. I.e. their template parameters must match up exactly. Can I constrain the attribute like this? Yes. You can check that with get_underlying_template. Or you could support the above by putting the attribute on the instantiation with the TEMPLATE_INFO for foo rather than a simple name. Jason
Re: [PATCH] Add gnu::diagnose_as attribute
On Tuesday, 22 June 2021 21:52:16 CEST Jason Merrill wrote: > For alias templates, you probably want the attribute only on the > templated class, not on the instantiations. Oh good point. My current patch does not allow the attribute on alias templates. Consider: template struct X {}; template using foo [[gnu::diagnose_as]] = X; I have no idea how this could work. I would have to set the attribute for an implicit partial specialization (not that I know of the existence of such a thing)? I.e. X would have to be diagnosed as foo, but X would have to be diagnosed as X, not foo. So if anything it should only support alias templates if they are strictly "renaming" the type. I.e. their template parameters must match up exactly. Can I constrain the attribute like this? Or should we rely on developers to be reasonable and only use it for template aliases with matching template params? -Matthias -- ── Dr. Matthias Kretz https://mattkretz.github.io GSI Helmholtz Centre for Heavy Ion Research https://gsi.de std::experimental::simd https://github.com/VcDevel/std-simd ──
Re: [PATCH] Add gnu::diagnose_as attribute
On 6/22/21 3:30 AM, Matthias Kretz wrote: On Wednesday, 16 June 2021 02:48:09 CEST Jason Merrill wrote: IIUC, your main concern is that my proposed diagnose_as *can* be used to make diagnostics worse, by replacing names with strings that are not valid identifiers. Of course, whoever uses the attribute to that effect should have a good reason to do so. Is your other concern that using the attribute in a "good" way is repetitive? Would you be happier if I make the string argument to the attribute optional for type aliases? Yes, and namespace aliases. I'll look into making the attribute argument optional for aliases. Would you accept the patch with this change? Yes (after resolving any technical details, of course). Questions: 1. If a type alias applies the attribute after a type was completed / implicitly instantiated (and possibly already used in diagnostics) should / can I still modify the type and add the attribute? Yes, because it has no semantic effect. For alias templates, you probably want the attribute only on the templated class, not on the instantiations. 2. About the namespace aliases: IIUC an attribute would currently be rejected because of the C++ grammar. Do you want to make it valid before WG21 officially decides how to proceed? And if you have a pointer for me where I'd have to adjust the grammar rules, that'd help. :) You will want to adjust cp_parser_namespace_alias_definition to handle attributes like cp_parser_namespace_definition. The latter currently accepts attributes both before and after the name, which seems like a good pattern to follow so it doesn't matter which WG21 chooses. Probably best to pedwarn about C++11 attributes in both locations for now, not just after. Jason
Re: [PATCH] Add gnu::diagnose_as attribute
On Wednesday, 16 June 2021 02:48:09 CEST Jason Merrill wrote: > > IIUC, your main concern is that my proposed diagnose_as *can* be used to > > make diagnostics worse, by replacing names with strings that are not > > valid identifiers. Of course, whoever uses the attribute to that effect > > should have a good reason to do so. Is your other concern that using the > > attribute in a "good" way is repetitive? Would you be happier if I make > > the string argument to the attribute optional for type aliases? > > Yes, and namespace aliases. I'll look into making the attribute argument optional for aliases. Would you accept the patch with this change? Questions: 1. If a type alias applies the attribute after a type was completed / implicitly instantiated (and possibly already used in diagnostics) should / can I still modify the type and add the attribute? 2. About the namespace aliases: IIUC an attribute would currently be rejected because of the C++ grammar. Do you want to make it valid before WG21 officially decides how to proceed? And if you have a pointer for me where I'd have to adjust the grammar rules, that'd help. :) Best, Matthias -- ── Dr. Matthias Kretz https://mattkretz.github.io GSI Helmholtz Centre for Heavy Ion Research https://gsi.de std::experimental::simd https://github.com/VcDevel/std-simd ──
Re: [PATCH] Add gnu::diagnose_as attribute
On 6/15/21 4:56 PM, Matthias Kretz wrote: On Tuesday, 15 June 2021 17:51:20 CEST Jason Merrill wrote: On 6/11/21 6:01 AM, Matthias Kretz wrote: For reference I'll attach my stdx::simd diagnose_as patch. We could also talk about extending the feature to provide more information about the diagnose_as substition. E.g. print a list of all diagnose_as substitutions, which were used, at the end of the output stream. Or simpler, print "note: some identifiers were simplified, use -fno-diagnostics-use- aliases to see their real names". Or perhaps before the first use of a name that doesn't correspond to a source-level name. Right. I guess that would be even easier to implement than printing it at the end. -struct _Scalar; + struct [[__gnu__::__diagnose_as__("scalar")]] _Scalar; template - struct _Fixed; + struct [[__gnu__::__diagnose_as__("fixed_size")]] _Fixed; Thes two could be the variant of the attribute without an explicit string, attached to the alias-declaration. Agreed. (since you don't have implementation concerns...) +using __sse [[__gnu__::__diagnose_as__("[SSE]")]] = _VecBuiltin<16>; +using __avx [[__gnu__::__diagnose_as__("[AVX]")]] = _VecBuiltin<32>; +using __avx512 [[__gnu__::__diagnose_as__("[AVX512]")]] = _VecBltnBtmsk<64>; + using __odr_helper [[__gnu__::__diagnose_as__("[ODR helper]")]] These [] names seem like minimal improvements over the __ names that you would get from the attribute without an explicit string. Right. It would, however, give the user an identifier that I don't want them to use in their code. We could argue "it has a double-underscore and it's not a documented implementation-defined type, so you're shooting yourself in the foot". Or we could just avoid the issue altogether. I agree this is not a huge issue. + inline namespace parallelism_v2 [[__gnu__::__diagnose_as__("std\u2093")]] { This could go on std::experimental itself, along with my proposed change to hide inline namespaces by default (with a note similar to the one above). Yes, with the following consequences: * If only the std::experimental::parallelism_v2::simd headers set the diagnose_as attribute on std::experimental, the #inclusion of changes the diagnostics of all other TS implementations. * If all TS implementations set the diagnose_as attribute, then it's basically impossible to go back to the long and scary name. Which is what we really should do as soon as there's both a std::simd and a stdₓ::simd. Attaching the diagnose_as attribute to the inline namespace allows for better granularity, even if it's maybe not good enough for some TSs. * If `namespace std { namespace experimental [[gnu::diagnose_as("foo")]] {` turns the scope into 'foo::' and not 'std::foo::' (not sure what you intended) then I could still attach the attribute to the inline namespace. So, yes, I could improve stdx::simd with what you propose. IMHO it wouldn't be as good as what I can do with the patch at hand, though. IIUC, your main concern is that my proposed diagnose_as *can* be used to make diagnostics worse, by replacing names with strings that are not valid identifiers. Of course, whoever uses the attribute to that effect should have a good reason to do so. Is your other concern that using the attribute in a "good" way is repetitive? Would you be happier if I make the string argument to the attribute optional for type aliases? Yes, and namespace aliases. Jason
Re: [PATCH] Add gnu::diagnose_as attribute
On Tuesday, 15 June 2021 17:51:20 CEST Jason Merrill wrote: > On 6/11/21 6:01 AM, Matthias Kretz wrote: > > For reference I'll attach my stdx::simd diagnose_as patch. > > > > We could also talk about extending the feature to provide more information > > about the diagnose_as substition. E.g. print a list of all diagnose_as > > substitutions, which were used, at the end of the output stream. Or > > simpler, print "note: some identifiers were simplified, use > > -fno-diagnostics-use- aliases to see their real names". > > Or perhaps before the first use of a name that doesn't correspond to a > source-level name. Right. I guess that would be even easier to implement than printing it at the end. > > -struct _Scalar; > > + struct [[__gnu__::__diagnose_as__("scalar")]] _Scalar; > > > > template > > > > - struct _Fixed; > > + struct [[__gnu__::__diagnose_as__("fixed_size")]] _Fixed; > > Thes two could be the variant of the attribute without an explicit > string, attached to the alias-declaration. Agreed. (since you don't have implementation concerns...) > > +using __sse [[__gnu__::__diagnose_as__("[SSE]")]] = _VecBuiltin<16>; > > +using __avx [[__gnu__::__diagnose_as__("[AVX]")]] = _VecBuiltin<32>; > > +using __avx512 [[__gnu__::__diagnose_as__("[AVX512]")]] = > > _VecBltnBtmsk<64>; + using __odr_helper [[__gnu__::__diagnose_as__("[ODR > > helper]")]] > These [] names seem like minimal improvements over the __ names that you > would get from the attribute without an explicit string. Right. It would, however, give the user an identifier that I don't want them to use in their code. We could argue "it has a double-underscore and it's not a documented implementation-defined type, so you're shooting yourself in the foot". Or we could just avoid the issue altogether. I agree this is not a huge issue. > > + inline namespace parallelism_v2 > > [[__gnu__::__diagnose_as__("std\u2093")]] { > This could go on std::experimental itself, along with my proposed change > to hide inline namespaces by default (with a note similar to the one above). Yes, with the following consequences: * If only the std::experimental::parallelism_v2::simd headers set the diagnose_as attribute on std::experimental, the #inclusion of changes the diagnostics of all other TS implementations. * If all TS implementations set the diagnose_as attribute, then it's basically impossible to go back to the long and scary name. Which is what we really should do as soon as there's both a std::simd and a stdₓ::simd. Attaching the diagnose_as attribute to the inline namespace allows for better granularity, even if it's maybe not good enough for some TSs. * If `namespace std { namespace experimental [[gnu::diagnose_as("foo")]] {` turns the scope into 'foo::' and not 'std::foo::' (not sure what you intended) then I could still attach the attribute to the inline namespace. So, yes, I could improve stdx::simd with what you propose. IMHO it wouldn't be as good as what I can do with the patch at hand, though. IIUC, your main concern is that my proposed diagnose_as *can* be used to make diagnostics worse, by replacing names with strings that are not valid identifiers. Of course, whoever uses the attribute to that effect should have a good reason to do so. Is your other concern that using the attribute in a "good" way is repetitive? Would you be happier if I make the string argument to the attribute optional for type aliases? -- ── Dr. Matthias Kretz https://mattkretz.github.io GSI Helmholtz Centre for Heavy Ion Research https://gsi.de std::experimental::simd https://github.com/VcDevel/std-simd ──
Re: [PATCH] Add gnu::diagnose_as attribute
On 6/11/21 6:01 AM, Matthias Kretz wrote: How can we make progress here? I could try to produce some "Tony Tables" of diagnostic output of my modified stdx::simd. I believe it's a major productivity boost to see abbreviated / "obfuscated" diagnostics *out-of-the box* (with the possibility to opt-out). Actually, it already *is* a productivity boost to me. Understanding diagnostics has improved from "1. ooof, I'm not going to read this, let me rather guess what the issue was 2. sh** I have to read it 3. several minutes later: I finally found the five words to understand the problem; I could use a break" to "1. right, let me check that" That's great. For reference I'll attach my stdx::simd diagnose_as patch. We could also talk about extending the feature to provide more information about the diagnose_as substition. E.g. print a list of all diagnose_as substitutions, which were used, at the end of the output stream. Or simpler, print "note: some identifiers were simplified, use -fno-diagnostics-use- aliases to see their real names". Or perhaps before the first use of a name that doesn't correspond to a source-level name. On Tuesday, 1 June 2021 21:12:18 CEST Jason Merrill wrote: Right, but then two of my design goals can't be met: 1. Diagnostics have an improved signal-to-noise ratio out of the box. 2. We can use replacement names that are not valid identifiers. This is the basic disconnect: I think that these goals are contradictory, and that replacement names that are not valid identifiers will just confuse users that don't know about them. If a user sees stdx::foo in a diagnostic and then tries to refer to stdx::foo and gets an error, the diagnostic is not more helpful than one that uses the fully qualified name. Jonathan, David, any thoughts on this issue? -struct _Scalar; + struct [[__gnu__::__diagnose_as__("scalar")]] _Scalar; template - struct _Fixed; + struct [[__gnu__::__diagnose_as__("fixed_size")]] _Fixed; Thes two could be the variant of the attribute without an explicit string, attached to the alias-declaration. +using __sse [[__gnu__::__diagnose_as__("[SSE]")]] = _VecBuiltin<16>; +using __avx [[__gnu__::__diagnose_as__("[AVX]")]] = _VecBuiltin<32>; +using __avx512 [[__gnu__::__diagnose_as__("[AVX512]")]] = _VecBltnBtmsk<64>; + using __odr_helper [[__gnu__::__diagnose_as__("[ODR helper]")]] These [] names seem like minimal improvements over the __ names that you would get from the attribute without an explicit string. + inline namespace parallelism_v2 [[__gnu__::__diagnose_as__("std\u2093")]] { This could go on std::experimental itself, along with my proposed change to hide inline namespaces by default (with a note similar to the one above). Jason
Re: [PATCH] Add gnu::diagnose_as attribute
How can we make progress here? I could try to produce some "Tony Tables" of diagnostic output of my modified stdx::simd. I believe it's a major productivity boost to see abbreviated / "obfuscated" diagnostics *out-of-the box* (with the possibility to opt-out). Actually, it already *is* a productivity boost to me. Understanding diagnostics has improved from "1. ooof, I'm not going to read this, let me rather guess what the issue was 2. sh** I have to read it 3. several minutes later: I finally found the five words to understand the problem; I could use a break" to "1. right, let me check that" For reference I'll attach my stdx::simd diagnose_as patch. We could also talk about extending the feature to provide more information about the diagnose_as substition. E.g. print a list of all diagnose_as substitutions, which were used, at the end of the output stream. Or simpler, print "note: some identifiers were simplified, use -fno-diagnostics-use- aliases to see their real names". On Tuesday, 1 June 2021 21:12:18 CEST Jason Merrill wrote: > > Right, but then two of my design goals can't be met: > > > > 1. Diagnostics have an improved signal-to-noise ratio out of the box. > > > > 2. We can use replacement names that are not valid identifiers. > > This is the basic disconnect: I think that these goals are > contradictory, and that replacement names that are not valid identifiers > will just confuse users that don't know about them. > > If a user sees stdx::foo in a diagnostic and then tries to refer to > stdx::foo and gets an error, the diagnostic is not more helpful than one > that uses the fully qualified name. > > Jonathan, David, any thoughts on this issue? -- ── Dr. Matthias Kretz https://mattkretz.github.io GSI Helmholtz Centre for Heavy Ion Research https://gsi.de std::experimental::simd https://github.com/VcDevel/std-simd ── diff --git a/libstdc++-v3/include/experimental/bits/simd.h b/libstdc++-v3/include/experimental/bits/simd.h index 43331134301..8e0cceff860 100644 --- a/libstdc++-v3/include/experimental/bits/simd.h +++ b/libstdc++-v3/include/experimental/bits/simd.h @@ -80,13 +80,13 @@ using __m512d [[__gnu__::__vector_size__(64)]] = double; using __m512i [[__gnu__::__vector_size__(64)]] = long long; #endif -namespace simd_abi { +namespace simd_abi [[__gnu__::__diagnose_as__("simd_abi")]] { // simd_abi forward declarations {{{ // implementation details: -struct _Scalar; + struct [[__gnu__::__diagnose_as__("scalar")]] _Scalar; template - struct _Fixed; + struct [[__gnu__::__diagnose_as__("fixed_size")]] _Fixed; // There are two major ABIs that appear on different architectures. // Both have non-boolean values packed into an N Byte register @@ -105,28 +105,11 @@ template template struct _VecBltnBtmsk; -template - using _VecN = _VecBuiltin; - -template - using _Sse = _VecBuiltin<_UsedBytes>; - -template - using _Avx = _VecBuiltin<_UsedBytes>; - -template - using _Avx512 = _VecBltnBtmsk<_UsedBytes>; - -template - using _Neon = _VecBuiltin<_UsedBytes>; - -// implementation-defined: -using __sse = _Sse<>; -using __avx = _Avx<>; -using __avx512 = _Avx512<>; -using __neon = _Neon<>; -using __neon128 = _Neon<16>; -using __neon64 = _Neon<8>; +#if defined __i386__ || defined __x86_64__ +using __sse [[__gnu__::__diagnose_as__("[SSE]")]] = _VecBuiltin<16>; +using __avx [[__gnu__::__diagnose_as__("[AVX]")]] = _VecBuiltin<32>; +using __avx512 [[__gnu__::__diagnose_as__("[AVX512]")]] = _VecBltnBtmsk<64>; +#endif // standard: template @@ -364,7 +347,7 @@ namespace __detail * users link TUs compiled with different flags. This is especially important * for using simd in libraries. */ - using __odr_helper + using __odr_helper [[__gnu__::__diagnose_as__("[ODR helper]")]] = conditional_t<__machine_flags() == 0, _OdrEnforcer, _MachineFlagsTemplate<__machine_flags(), __floating_point_flags()>>; @@ -689,7 +672,7 @@ template __is_avx512_abi() { constexpr auto _Bytes = __abi_bytes_v<_Abi>; -return _Bytes <= 64 && is_same_v, _Abi>; +return _Bytes <= 64 && is_same_v, _Abi>; } // }}} diff --git a/libstdc++-v3/include/experimental/bits/simd_detail.h b/libstdc++-v3/include/experimental/bits/simd_detail.h index 78ad33f74e4..1f127cd0d52 100644 --- a/libstdc++-v3/include/experimental/bits/simd_detail.h +++ b/libstdc++-v3/include/experimental/bits/simd_detail.h @@ -36,7 +36,7 @@ {\ _GLIBCXX_BEGIN_NAMESPACE_VERSION \ namespace experimental { \ - inline namespace parallelism_v2 { + inline namespace parallelism_v2 [[__gnu__::__diagnose_as__("std\u209
Re: [PATCH] Add gnu::diagnose_as attribute
On Tuesday, 1 June 2021 21:12:18 CEST Jason Merrill wrote: > On 5/28/21 3:42 AM, Matthias Kretz wrote: > > On Friday, 28 May 2021 05:05:52 CEST Jason Merrill wrote: > >> I'd think you could get the same effect from a hypothetical > >> > >> namespace [[gnu::diagnose_as]] stdx = std::experimental; > >> > >> though we'll need to add support for attributes on namespace aliases to > >> the grammar. > > > > Right, but then two of my design goals can't be met: > > > > 1. Diagnostics have an improved signal-to-noise ratio out of the box. > > > > 2. We can use replacement names that are not valid identifiers. > > This is the basic disconnect: I think that these goals are > contradictory, and that replacement names that are not valid identifiers > will just confuse users that don't know about them. With signal-to-noise ratio I meant the ratio (averaged over all GCC users - so yes, we can't give actual numbers for these): #characters one needs to read to understand / #total diagnostic characters. Or more specifically 1 - #characters that are distracting from understanding the issue / #total diagnostic characters. Consider that for the stdx::simd case I regularly hit the problem that vim's QuickFix truncates at 4095 characters and the message basically just got started (i.e. it's sometimes impossible to use vim's QuickFix to understand errors involving stdx::simd). There's *a lot* of noise that must be removed *per default*. WRT "invalid identifiers", there are two types: (1) string of characters that is not a valid C++ identifier (2) valid C++ identifier, but not defined for the given TU (2) can be confusing, I agree, but doesn't have to be. (1) provides a stronger hint that something is either abbreviated or intentionally hidden from the user. If I write `std::experimental::simd` in my code and get a diagnostic that says 'stdₓ::simd' then it's relatively easy to make the connection what happened here: 'stdₓ' clearly must mean something else than a literal 'stdₓ' in my code. The user knows there's no `std::simd' so it must be `std::experimental::simd`. (Note that once std::experimental::simd goes into the IS, I would be the first to propose a change for 'stdₓ::simd' back to 'std::experimental::simd'.) > If a user sees stdx::foo in a diagnostic and then tries to refer to > stdx::foo and gets an error, the diagnostic is not more helpful than one > that uses the fully qualified name. Hmm, if GCC prints an actual suggestion like "write 'stdₓ::foo' here" then yes, I agree. That should not make use of diagnose_as. > Jonathan, David, any thoughts on this issue? > > > I can imagine using it to make _Internal __names more readable while at > > the > > same time discouraging users to utter them in their own code. Sorry for > > the > > bad code obfuscation example above. > > > > An example for consideration from stdx::simd: > >namespace std { > >namespace experimental { > >namespace parallelism_v2 [[gnu::diagnose_as("stdx")]] { > >namespace simd_abi [[gnu::diagnose_as("simd_abi")]] { > > > > template > > > >struct _VecBuiltin; > > > > template > > > >struct _VecBltnBtmsk; > > > >#if x86 > > > > using __ignore_me_0 [[gnu::diagnose_as("[SSE]")]] = _VecBuiltin<16>; > > using __ignore_me_1 [[gnu::diagnose_as("[AVX]")]] = _VecBuiltin<32>; > > using __ignore_me_2 [[gnu::diagnose_as("[AVX512]")]] = > > _VecBltnBtmsk<64>; > > > >#endif > > > > > > Then diagnostics would print 'stdx::simd' > > instead of 'stdx::simd>'. (Users utter > > the type by saying e.g. 'stdx::native_simd', while compiling with > > AVX512 flags.) > > Wouldn't it be better to print stdx::native_simd if that's how > the users write the type? No. For example, I might expect that native_simd maps to AVX-512 vectors but forgot the relevant -m flag(s). If the diagnostics show 'simd' I have a good chance of catching that issue. And the other way around: If I wrote `stdx::simd` and it happens to be the same type as the native_simd typedef, it would show the latter in diagnostics. Similar issue with asking for a simd ABI with `simd_abi::deduce_t`: I typically don't want to know whether that's also native_simd but rather what exact simd_abi I got. And no, as a user I don't typically care about the libstdc++ implementation details but what those details mean. -- ── Dr. Matthias Kretz https://mattkretz.github.io GSI Helmholtz Centre for Heavy Ion Research https://gsi.de std::experimental::simd https://github.com/VcDevel/std-simd ──
Re: [PATCH] Add gnu::diagnose_as attribute
On 5/28/21 3:42 AM, Matthias Kretz wrote: On Friday, 28 May 2021 05:05:52 CEST Jason Merrill wrote: On 5/27/21 6:07 PM, Matthias Kretz wrote: On Thursday, 27 May 2021 23:15:46 CEST Jason Merrill wrote: On 5/27/21 2:54 PM, Matthias Kretz wrote: 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. As long as you ignore the line numbers, it's a puzzling diagnostic. Only briefly puzzling, I think; Vir::A is a valid way of referring to both types. True. But that's also what lead to the error. GCC easily clears it up nowadays, but wouldn't anymore if inline namespaces were hidden by default. I'd think you could get the same effect from a hypothetical namespace [[gnu::diagnose_as]] stdx = std::experimental; though we'll need to add support for attributes on namespace aliases to the grammar. Right, but then two of my design goals can't be met: 1. Diagnostics have an improved signal-to-noise ratio out of the box. 2. We can use replacement names that are not valid identifiers. This is the basic disconnect: I think that these goals are contradictory, and that replacement names that are not valid identifiers will just confuse users that don't know about them. If a user sees stdx::foo in a diagnostic and then tries to refer to stdx::foo and gets an error, the diagnostic is not more helpful than one that uses the fully qualified name. Jonathan, David, any thoughts on this issue? I don't think libstdc++ would ship with a namespace alias outside of the std namespace. So we'd place the "burden" of using diagnose_as correctly on our users. Also as a user you'd possibly have to repeat the namespace alias in every source file and/or place it in your applications/libraries namespace. 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. Yes, but you could also use it to apply diagnose_as to a template instantiation without introducing a name for users. E.g. using __only_to_apply_the_attribute [[gnu::diagnose_as("intvector")]] = std::vector; Now all diagnostics of 'std::vector' would print 'intvector' instead. Yes, but why would you want to? Making diagnostics print names that the user can't use in their own code seems obfuscatory, and requiring users to write the same names in two places seems like extra work. I can imagine using it to make _Internal __names more readable while at the same time discouraging users to utter them in their own code. Sorry for the bad code obfuscation example above. An example for consideration from stdx::simd: namespace std { namespace experimental { namespace parallelism_v2 [[gnu::diagnose_as("stdx")]] { namespace simd_abi [[gnu::diagnose_as("simd_abi")]] { template struct _VecBuiltin; template struct _VecBltnBtmsk; #if x86 using __ignore_me_0 [[gnu::diagnose_as("[SSE]")]] = _VecBuiltin<16>; using __ignore_me_1 [[gnu::diagnose_as("[AVX]")]] = _VecBuiltin<32>; using __ignore_me_2 [[gnu::diagnose_as("[AVX512]")]] = _VecBltnBtmsk<64>; #endif Then diagnostics would print 'stdx::simd' instead of 'stdx::simd>'. (Users utter the type by saying e.g. 'stdx::native_simd', while compiling with AVX512 flags.) Wouldn't it be better to print stdx::native_simd if that's how the users write the type? But in general, I tend to agree, for type aliases there's rarely a case where the names wouldn't match. However, I didn't want to special-case the attribute parameters for type aliases (or introduce another attribute just for this case). The attribute works consistently and with the same interface independent of where it's used. I tried to build a generic, broad feature instead of a narrow one-problem solution. "Treat this declaration as the name of the type/namespace it refers to in diagnostics" also seems consistent to me. Sure. In general, I think namespace foo [[gnu::this_is_the_name_I_want]] = bar; using foo [[gnu::this_is_the_name_I_want]] = bar; is not a terribly bad idea on its own. But it's not the solution for the problems I set out to solve. Still, perhaps it would be better to store these aliases in a separate hash table instead of *_ATTRIBUTES. Maybe. For performance reasons or for simplification of the implementation? I was thinking for not messing with the type after it's defined, but there are other things that do that (such as lazy declaration of implicit constructors) so I wouldn't worry about it. Jason
Re: [PATCH] Add gnu::diagnose_as attribute
On Friday, 28 May 2021 05:05:52 CEST Jason Merrill wrote: > On 5/27/21 6:07 PM, Matthias Kretz wrote: > > On Thursday, 27 May 2021 23:15:46 CEST Jason Merrill wrote: > >> On 5/27/21 2:54 PM, Matthias Kretz wrote: > >>> 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. > > > > As long as you ignore the line numbers, it's a puzzling diagnostic. > > Only briefly puzzling, I think; Vir::A is a valid way of referring to > both types. True. But that's also what lead to the error. GCC easily clears it up nowadays, but wouldn't anymore if inline namespaces were hidden by default. > I'd think you could get the same effect from a hypothetical > > namespace [[gnu::diagnose_as]] stdx = std::experimental; > > though we'll need to add support for attributes on namespace aliases to > the grammar. Right, but then two of my design goals can't be met: 1. Diagnostics have an improved signal-to-noise ratio out of the box. 2. We can use replacement names that are not valid identifiers. I don't think libstdc++ would ship with a namespace alias outside of the std namespace. So we'd place the "burden" of using diagnose_as correctly on our users. Also as a user you'd possibly have to repeat the namespace alias in every source file and/or place it in your applications/libraries namespace. > >> 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. > > > > Yes, but you could also use it to apply diagnose_as to a template > > instantiation without introducing a name for users. E.g. > > > >using __only_to_apply_the_attribute [[gnu::diagnose_as("intvector")]] > > > > = std::vector; > > > > Now all diagnostics of 'std::vector' would print 'intvector' instead. > > Yes, but why would you want to? Making diagnostics print names that the > user can't use in their own code seems obfuscatory, and requiring users > to write the same names in two places seems like extra work. I can imagine using it to make _Internal __names more readable while at the same time discouraging users to utter them in their own code. Sorry for the bad code obfuscation example above. An example for consideration from stdx::simd: namespace std { namespace experimental { namespace parallelism_v2 [[gnu::diagnose_as("stdx")]] { namespace simd_abi [[gnu::diagnose_as("simd_abi")]] { template struct _VecBuiltin; template struct _VecBltnBtmsk; #if x86 using __ignore_me_0 [[gnu::diagnose_as("[SSE]")]] = _VecBuiltin<16>; using __ignore_me_1 [[gnu::diagnose_as("[AVX]")]] = _VecBuiltin<32>; using __ignore_me_2 [[gnu::diagnose_as("[AVX512]")]] = _VecBltnBtmsk<64>; #endif Then diagnostics would print 'stdx::simd' instead of 'stdx::simd>'. (Users utter the type by saying e.g. 'stdx::native_simd', while compiling with AVX512 flags.) > > But in general, I tend to agree, for type aliases there's rarely a case > > where the names wouldn't match. > > > > However, I didn't want to special-case the attribute parameters for type > > aliases (or introduce another attribute just for this case). The attribute > > works consistently and with the same interface independent of where it's > > used. I tried to build a generic, broad feature instead of a narrow > > one-problem solution. > > "Treat this declaration as the name of the type/namespace it refers to > in diagnostics" also seems consistent to me. Sure. In general, I think namespace foo [[gnu::this_is_the_name_I_want]] = bar; using foo [[gnu::this_is_the_name_I_want]] = bar; is not a terribly bad idea on its own. But it's not the solution for the problems I set out to solve. > Still, perhaps it would be better to store these aliases in a separate hash > table instead of *_ATTRIBUTES. Maybe. For performance reasons or for simplification of the implementation? What entity could I use for hashing? The identifier alone wouldn't suffice since different instantiations of the same class template can have different diagnose_as values (e.g. std::string, std::wstring, ...). -- ── Dr. Matthias Kretz https://mattkretz.github.io GSI Helmholtz Centre for Heavy Ion Research https://gsi.de std::experimental::simd https://github.com/VcDevel/std-simd ──
Re: [PATCH] Add gnu::diagnose_as attribute
On 5/27/21 6:07 PM, Matthias Kretz wrote: On Thursday, 27 May 2021 23:15:46 CEST Jason Merrill wrote: On 5/27/21 2:54 PM, Matthias Kretz wrote: 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. As long as you ignore the line numbers, it's a puzzling diagnostic. Only briefly puzzling, I think; Vir::A is a valid way of referring to both types. 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. True for std::string, not true for TS's where the extra '::experimental' still makes finding the relevant information in diagnostics harder than necessary. And it seems like applying the attribute to a namespace means that enclosing namespaces are not printed, unlike the handling for types. Yes, that's also how I documented it. For nested namespaces I wanted to enable the removal of nesting (e.g. from std::experimental::parallelism_v2::simd to stdx::simd). However, for types and functions it would be a problem to drop the enclosing scope, because the scope can be class templates and thus the diagnose_as attribute would remove all template parms & args. I'd think you could get the same effect from a hypothetical namespace [[gnu::diagnose_as]] stdx = std::experimental; though we'll need to add support for attributes on namespace aliases to the grammar. - typedef basic_stringstring; + 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. Yes, but you could also use it to apply diagnose_as to a template instantiation without introducing a name for users. E.g. using __only_to_apply_the_attribute [[gnu::diagnose_as("intvector")]] = std::vector; Now all diagnostics of 'std::vector' would print 'intvector' instead. Yes, but why would you want to? Making diagnostics print names that the user can't use in their own code seems obfuscatory, and requiring users to write the same names in two places seems like extra work. But in general, I tend to agree, for type aliases there's rarely a case where the names wouldn't match. However, I didn't want to special-case the attribute parameters for type aliases (or introduce another attribute just for this case). The attribute works consistently and with the same interface independent of where it's used. I tried to build a generic, broad feature instead of a narrow one-problem solution. "Treat this declaration as the name of the type/namespace it refers to in diagnostics" also seems consistent to me. FWIW, before you suggest to have one attribute for namespaces and one for type aliases (to cover the std::string case), I have another use case in stdx::simd (the spec requires simd_abi::scalar to be an alias): namespace std::experimental::parallelism_v2::simd_abi { struct [[gnu::diagnose_as("scalar")]] _Scalar; using scalar = _Scalar; } If the attribute were on the type alias (using scalar [[gnu::diagnose_as]] = _Scalar;), then we'd have to apply the attribute to _Scalar after it was completed. That seemed like a bad idea to me. Well, in your sample code, _Scalar isn't complete at the point of the alias-declaration. But even if it were, since the attribute doesn't affect code generation, it doesn't strike me as a big problem. Still, perhaps it would be better to store these aliases in a separate hash table instead of *_ATTRIBUTES. Jason
Re: [PATCH] Add gnu::diagnose_as attribute
On Thursday, 27 May 2021 23:15:46 CEST Jason Merrill wrote: > On 5/27/21 2:54 PM, Matthias Kretz wrote: > > 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. As long as you ignore the line numbers, it's a puzzling diagnostic. > > 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. True for std::string, not true for TS's where the extra '::experimental' still makes finding the relevant information in diagnostics harder than necessary. > And it seems like applying the attribute to a > namespace means that enclosing namespaces are not printed, unlike the > handling for types. Yes, that's also how I documented it. For nested namespaces I wanted to enable the removal of nesting (e.g. from std::experimental::parallelism_v2::simd to stdx::simd). However, for types and functions it would be a problem to drop the enclosing scope, because the scope can be class templates and thus the diagnose_as attribute would remove all template parms & args. > > - typedef basic_stringstring; > > + 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. Yes, but you could also use it to apply diagnose_as to a template instantiation without introducing a name for users. E.g. using __only_to_apply_the_attribute [[gnu::diagnose_as("intvector")]] = std::vector; Now all diagnostics of 'std::vector' would print 'intvector' instead. But in general, I tend to agree, for type aliases there's rarely a case where the names wouldn't match. However, I didn't want to special-case the attribute parameters for type aliases (or introduce another attribute just for this case). The attribute works consistently and with the same interface independent of where it's used. I tried to build a generic, broad feature instead of a narrow one-problem solution. FWIW, before you suggest to have one attribute for namespaces and one for type aliases (to cover the std::string case), I have another use case in stdx::simd (the spec requires simd_abi::scalar to be an alias): namespace std::experimental::parallelism_v2::simd_abi { struct [[gnu::diagnose_as("scalar")]] _Scalar; using scalar = _Scalar; } If the attribute were on the type alias (using scalar [[gnu::diagnose_as]] = _Scalar;), then we'd have to apply the attribute to _Scalar after it was completed. That seemed like a bad idea to me. -- ── Dr. Matthias Kretz https://mattkretz.github.io GSI Helmholtz Centre for Heavy Ion Research https://gsi.de std::experimental::simd https://github.com/VcDevel/std-simd ──
Re: [PATCH] Add gnu::diagnose_as attribute
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_stringstring; + 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)) + { +
Re: [PATCH] Add gnu::diagnose_as attribute
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' > > 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"))) { } } 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_stringstring; + typedef basic_string string [[__gnu__::__diagnose_as__("string")]]; #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); +
Re: [PATCH] Add gnu::diagnose_as attribute
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? 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? Jason
Re: [PATCH] Add gnu::diagnose_as attribute
On Friday, 14 May 2021 18:05:03 CEST Martin Sebor wrote: > [...] > > At the same time, my concern with adding another syntactic renaming > mechanism that's specifically intended to change symbol names in > diagnostics (and the two macros) but nowhere else is that it would > considerably raise the risk of confusion between the name in > the source and the name in the diagnostic. (E.g., one source > file renames symbol Foo to Bar but another one doesn't; messages > from one file will refer to Foo and other to Bar with no way of > knowing they're the same. This could be solved by printing both > the renamed symbol and what it stands for (like for typedefs or > template instantiations) but that would then increase the S/R > ratio this solution is aimed at improving. Providing an option > to disable the renaming is great but suffers from the same problem: > to be sure what symbol is being referred to, users would have to > disable the renaming. I agree with your concern. This attribute is easy to use for obfuscation and slightly harder to use in a significantly helpful way. If you've ever had to parse diagnostic output involving std::experimental::parallelism_v2::simd (when your source says ``` namespace stdx = std::experimental; stdx::simd ``` you know the potential of this attribute. > It doesn't seem like we can have it both ways. But maybe indicating > in some way when a symbol mentioned in a diagnostic is the result of > renaming by the attribute, without spelling out the original name, > would be a good enough compromise. Though that won't help with > the same problem in the expansion of macros like __FUNCTION__. I've been thinking about it. It would not be helpful to always display the original names when diagnose_as overrides something. But maybe there could be like a glossary at the bottom of the diagnostic output? Or simply a "note: Some names above do not reflect their real names in the source. Use -fno- diagnostics-use-aliases to disable the replacement." > Other than that, I have a couple of questions: > > Are there any implementations that provide this attribute? (If > so does the syntax and effects match? It would be nice to be > compatible if possible.) There exists nothing like it AFAIK. > Does the attribute change the typeinfo strings? If not, did you > consider the interplay between compile-time and runtime diagnostics > involving names decorated with the attribute? Good question. From all my tests (and my understanding how typeinfo strings are construted) the attribute has no effect on it. From one of my tests: `X0::X3` is diagnosed as "[int|int]::X.3", and `typeid(X0::X3).name()` is "N2X0IiiE2X3E" which is "X0::X3" again. I experienced the difference between compile-time and runtime diagnostics myself (i.e. objdump and gdb disagree with diagnostic output) when working on stdx::simd (I've been using the attribute since March with stdx::simd). > (A related concern > is the runtime output of messages involving macros like > __FUNCTION__ issued from object files compiled by different > compilers or versions of the same compiler.) Right, if you make __FUNCTION__ part of your ABI or communication protocol then the attribute can create incompatibilities. I'm not sure we should care about this part too much. Note that -fpretty-templates also changes the __PRETTY_FUNCTION__ string. While I plan to submit a patch for std::__cxx11::basic_string to e.g. turn '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]' into '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)' , i.e. affecting close to all C++ users, I don't believe the accumulated headache will increase. ;-) > > [...] > > Other diagnostics involving attributes do not start with an article. > Can you please drop the "the"? (In general, I would suggest to either > reuse or follow the style of existing diagnostics issued from the same > file, and/or look at those in gcc/po/gcc.pot. Not nearly all of then > are consistent with one another but it's best to avoid introducing > yet another style; it will make converging on a single style easier, > and reduces the effort involved in translating messages). > [...] Will do. Thanks for your detailed comments on this topic. Very helpful 👍. > > + continue; > > + } > > + if (TREE_CODE (TREE_VALUE (args)) != STRING_CST) > > + { > > + error ("the argument to the %qE attribute must be a string " > > +
Re: [PATCH] Add gnu::diagnose_as attribute
On 5/4/21 5: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. There are other mechanisms to change symbol names such as macros, typedefs or alias or using declarations, and at the object level, ELF aliases, and even the C++ ABI with its substitutions for types like std::string and with the abi_tag attribute. They all have a potential to cause some confusion when a symbol with one name is referred to by another. For the syntactic mechanisms compilers usually deal with the problem by mentioning both the macro or alias name and its expansion or target in the diagnostics they issue. The renaming prescribed by the C++ is reflected in the mangled symbols that demanglers know how to decode and restore the their originals. For ELF aliases no such solution exists but they are used only rarely by well-tested system code and so the different names don't typically come up in compiler (or even linker) diagnostics. When they do, the names often share some common prefix or suffix so the confusion is minimal. I like the idea of printing names in diagnostics that are familiar to users while leaving out implementation details that's in most cases irrelevant to them. At the same time, my concern with adding another syntactic renaming mechanism that's specifically intended to change symbol names in diagnostics (and the two macros) but nowhere else is that it would considerably raise the risk of confusion between the name in the source and the name in the diagnostic. (E.g., one source file renames symbol Foo to Bar but another one doesn't; messages from one file will refer to Foo and other to Bar with no way of knowing they're the same. This could be solved by printing both the renamed symbol and what it stands for (like for typedefs or template instantiations) but that would then increase the S/R ratio this solution is aimed at improving. Providing an option to disable the renaming is great but suffers from the same problem: to be sure what symbol is being referred to, users would have to disable the renaming. It doesn't seem like we can have it both ways. But maybe indicating in some way when a symbol mentioned in a diagnostic is the result of renaming by the attribute, without spelling out the original name, would be a good enough compromise. Though that won't help with the same problem in the expansion of macros like __FUNCTION__. Other than that, I have a couple of questions: Are there any implementations that provide this attribute? (If so does the syntax and effects match? It would be nice to be compatible if possible.) Does the attribute change the typeinfo strings? If not, did you consider the interplay between compile-time and runtime diagnostics involving names decorated with the attribute? (A related concern is the runtime output of messages involving macros like __FUNCTION__ issued from object files compiled by different compilers or versions of the same compiler.) Below are a few comments on the changes below, mostly focused on the phrasing and style of diagnostic messages. As others already mentioned, the patch should include tests (including them early in the review process helps get an idea of how the whole feature works). 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. gcc/ChangeLog: PR c++/89370 * doc/extend.texi: Document the diagnose_as attribute. * doc/invoke.texi: Document -fno-diagnostics-use-aliases. gcc/c-family/ChangeLog: PR c++/89370 * c.opt (fdiagnostics-use-aliases): New diagnostics flag. gcc/cp/ChangeLog: PR c++/89370 * error.c (dump_scope): When printing the name of a namespace, look for the diagnose_as attribute. If found, print the associated string instead of calling dump_decl. (dump_decl_name_or_diagnose_as): New function to replace dump_decl (pp, DECL_NAME(t), flags) and inspect the tree for the diagnose_as attribute before printing the DECL_NAME. (dump_aggr_type): If the type has a diagnose_as attribute, print the associated string instead of printing the original type name. (dump_simple_decl): Call dump_decl_name_or_diagnose_as instead of dump_decl. (dump_decl): Ditto. (lang_decl_nam
Re: [PATCH] Add gnu::diagnose_as attribute
> On Tuesday, 4 May 2021 15:34:13 CEST David Malcolm wrote: > > Does the patch interact correctly with the %H and %I codes that try to > > show the differences between two template types? While looking into this, I noticed that given namespace std { struct A {}; typedef A B; } const std::B would print as "'const B' {aka 'const std::A'}", i.e. without printing the scope of the typedef. I traced it to cp/error.c (dump_type). In the `if (TYPE_P (t) && typedef_variant_p (t))` branch, in the final else branch only cv-qualifiers and identifier are printed: pp_cxx_cv_qualifier_seq (pp, t); pp_cxx_tree_identifier (pp, TYPE_IDENTIFIER (t)); I believe the following should go in between, correct? pp_cxx_cv_qualifier_seq (pp, t); if (! (flags & TFF_UNQUALIFIED_NAME)) dump_scope (pp, CP_DECL_CONTEXT (TYPE_NAME (t)), flags); pp_cxx_tree_identifier (pp, TYPE_IDENTIFIER (t)); This is important for my diagnose_as patch because otherwise the output is: 'const string' {aka 'const std::string'} which is confusing and unnecessarily verbose. Patch below. From: Matthias Kretz dump_type on 'const std::string' should not print 'const string' unless TFF_UNQUALIFIED_NAME is requested. gcc/cp/ChangeLog: * error.c: Call dump_scope when printing a typedef. --- gcc/cp/error.c | 2 ++ 1 file changed, 2 insertions(+) -- ── Dr. Matthias Kretz https://mattkretz.github.io GSI Helmholtz Centre for Heavy Ion Research https://gsi.de std::experimental::simd https://github.com/VcDevel/std-simd ── diff --git a/gcc/cp/error.c b/gcc/cp/error.c index 10b547afaa7..edeaad44bcd 100644 --- a/gcc/cp/error.c +++ b/gcc/cp/error.c @@ -511,6 +511,8 @@ dump_type (cxx_pretty_printer *pp, tree t, int flags) else { pp_cxx_cv_qualifier_seq (pp, t); + if (! (flags & TFF_UNQUALIFIED_NAME)) + dump_scope (pp, CP_DECL_CONTEXT (TYPE_NAME (t)), flags); pp_cxx_tree_identifier (pp, TYPE_IDENTIFIER (t)); return; }
Re: [PATCH] Add gnu::diagnose_as attribute
On Tue, 2021-05-04 at 16:23 +0200, Matthias Kretz wrote: > On Tuesday, 4 May 2021 15:34:13 CEST David Malcolm wrote: > > On Tue, 2021-05-04 at 13:13 +0200, Matthias Kretz wrote: > > > 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. > > > [...] > > > > Thanks for the patch, it looks very promising. > > Thanks. I'm new to modifying the compiler like this, so please be > extra > careful with my patch. I believe I understand most of what I did, but > I might > have misunderstood. :) That's tends to be how I feel when working on the C++ FE :) > > > 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 > > Right. If you know of an existing similar testcase, that'd help me a > lot to > get started. FWIW I implemented the %H and %I codes in f012c8ef4b35dcee9b5a3807868d050812d5b3b9 and that commit adds various DejaGnu testcases that exercise C++ diagnostics with and without various options, verifying the precise wording of errors. So that might be a good place to look. > > > Does the patch affect the output of labels when underlining ranges of > > source code in diagnostics? > > AFAIU (and tested), it doesn't affect source code output. So, no? Sorry, I was unclear. I was referring to this kind of thing: $ cat t.C #include std::string test (std::string s) { return &s; } $ g++ t.C t.C: In function ‘std::string test(std::string)’: t.C:5:12: error: could not convert ‘& s’ from ‘std::string*’ {aka ‘std::__cxx11::basic_string*’} to ‘std::string’ {aka ‘std::__cxx11::basic_string’} 5 | return &s; |^~ || |std::string* {aka std::__cxx11::basic_string*} i.e. the final line in the output above, where the underlined range of source code in line 5 is labelled, showing the type of the expression. Hopefully it ought to automatically just drop out of the work you've already done. FWIW an example of implementation this can be seen in a14feb3c783fba6af8d66b8138214a3a313be5c5 (which added labels for type errors in C++ binary operators). > Does the patch interact correctly with the %H and %I codes that try to > show the differences between two template types? I don't know. I'll try to find out. If you have a good idea (or pointer) for a testcase, let me know. See above. Dave
Re: [PATCH] Add gnu::diagnose_as attribute
On Tuesday, 4 May 2021 16:23:23 CEST Matthias Kretz wrote: > On Tuesday, 4 May 2021 15:34:13 CEST David Malcolm wrote: > > Does the patch interact correctly with the %H and %I codes that try to > > show the differences between two template types? > > I don't know. I'll try to find out. If you have a good idea (or pointer) for > a testcase, let me know. I see it now. It currently does not interact with %H and %I (at least in my tests). I'll investigate what it should do. -- ── Dr. Matthias Kretz https://mattkretz.github.io GSI Helmholtz Centre for Heavy Ion Research https://gsi.de std::experimental::simd https://github.com/VcDevel/std-simd ──
Re: [PATCH] Add gnu::diagnose_as attribute
On Tuesday, 4 May 2021 15:34:13 CEST David Malcolm wrote: > On Tue, 2021-05-04 at 13:13 +0200, Matthias Kretz wrote: > > 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. > > [...] > > Thanks for the patch, it looks very promising. Thanks. I'm new to modifying the compiler like this, so please be extra careful with my patch. I believe I understand most of what I did, but I might have misunderstood. :) > 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 Right. If you know of an existing similar testcase, that'd help me a lot to get started. > Does the patch affect the output of labels when underlining ranges of > source code in diagnostics? AFAIU (and tested), it doesn't affect source code output. So, no? > Does the patch interact correctly with the %H and %I codes that try to > show the differences between two template types? I don't know. I'll try to find out. If you have a good idea (or pointer) for a testcase, let me know. > I have some minor nits from a diagnostics point of view: > [...] > Please add an auto_diagnostic_group here so that the "inform" is > associated with the "error". > [...] > diagnose_as should be in quotes here (%< and %>). > [...] > Please quote extern "C": Thanks. All done in my tree. I'll work on testcases before sending an updated patch. > Thanks again for the patch; hope this is constructive 👍 -- ── Dr. Matthias Kretz https://mattkretz.github.io GSI Helmholtz Centre for Heavy Ion Research https://gsi.de std::experimental::simd https://github.com/VcDevel/std-simd ──
Re: [PATCH] Add gnu::diagnose_as attribute
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
[PATCH] Add gnu::diagnose_as attribute
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. gcc/ChangeLog: PR c++/89370 * doc/extend.texi: Document the diagnose_as attribute. * doc/invoke.texi: Document -fno-diagnostics-use-aliases. gcc/c-family/ChangeLog: PR c++/89370 * c.opt (fdiagnostics-use-aliases): New diagnostics flag. gcc/cp/ChangeLog: PR c++/89370 * error.c (dump_scope): When printing the name of a namespace, look for the diagnose_as attribute. If found, print the associated string instead of calling dump_decl. (dump_decl_name_or_diagnose_as): New function to replace dump_decl (pp, DECL_NAME(t), flags) and inspect the tree for the diagnose_as attribute before printing the DECL_NAME. (dump_aggr_type): If the type has a diagnose_as attribute, print the associated string instead of printing the original type name. (dump_simple_decl): Call dump_decl_name_or_diagnose_as instead of dump_decl. (dump_decl): Ditto. (lang_decl_name): Ditto. (dump_function_decl): Ensure complete replacement of the class template diagnostics if a diagnose_as attribute is present. (dump_function_name): Replace the function diagnostic output if the diagnose_as attribute is set. * name-lookup.c (handle_namespace_attrs): Handle the diagnose_as attribute. Ensure exactly one string argument. Ensure previous diagnose_as attributes used the same name. * tree.c (cxx_attribute_table): Add diagnose_as attribute to the table. (check_diagnose_as_redeclaration): New function; copied and adjusted from check_abi_tag_redeclaration. (handle_diagnose_as_attribute): New function; copied and adjusted from handle_abi_tag_attribute. If the given *node is a TYPE_DECL and the TREE_TYPE is an implicit class template instantiation, call decl_attributes to add the diagnose_as attribute to the TREE_TYPE. --- gcc/c-family/c.opt | 4 ++ gcc/cp/error.c | 85 --- gcc/cp/name-lookup.c | 27 ++ gcc/cp/tree.c| 117 +++ gcc/doc/extend.texi | 37 ++ gcc/doc/invoke.texi | 9 +++- 6 files changed, 270 insertions(+), 9 deletions(-) -- ── Dr. Matthias Kretz https://mattkretz.github.io GSI Helmholtz Centre for Heavy Ion Research https://gsi.de std::experimental::simd https://github.com/VcDevel/std-simd ──diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index 3f8b72cdc00..0cf01c6dba4 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -1582,6 +1582,10 @@ fdiagnostics-show-template-tree C++ ObjC++ Var(flag_diagnostics_show_template_tree) Init(0) Print hierarchical comparisons when template types are mismatched. +fdiagnostics-use-aliases +C++ Var(flag_diagnostics_use_aliases) Init(1) +Replace identifiers or scope names in diagnostics as defined by the diagnose_as attribute. + fdirectives-only C ObjC C++ ObjC++ Preprocess directives only. diff --git a/gcc/cp/error.c b/gcc/cp/error.c index c88d1749a0f..10b547afaa7 100644 --- a/gcc/cp/error.c +++ b/gcc/cp/error.c @@ -35,6 +35,7 @@ along with GCC; see the file COPYING3. If not see #include "internal-fn.h" #include "gcc-rich-location.h" #include "cp-name-hint.h" +#include "attribs.h" #define pp_separate_with_comma(PP) pp_cxx_separate_with (PP, ',') #define pp_separate_with_semicolon(PP) pp_cxx_separate_with (PP, ';') @@ -66,6 +67,7 @@ static void dump_alias_template_specialization (cxx_pretty_printer *, tree, int) static void dump_type (cxx_pretty_printer *, tree, int); static void dump_typename (cxx_pretty_printer *, tree, int); static void dump_simple_decl (cxx_pretty_printer *, tree, tree, int); +static void dump_decl_name_or_diagnose_as (cxx_pretty_printer *, tree, int); static void dump_decl (cxx_pretty_printer *, tree, int); static void dump_template_decl (cxx_pretty_printer *, tree, int);