Re: [PATCH] c++: error message for dependent template members [PR70417]
On 8/20/21 12:56 PM, Anthony Sharp via Gcc-patches wrote: Hi, hope everyone is well. I have a patch here for issue 70417 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70417). I'm still a GCC noob, and this is probably the hardest thing I have ever coded in my life, so please forgive any initial mistakes! TLDR This patch introduces a helpful error message when the user attempts to put a template-id after a member access token (., -> or ::) in a dependent context without having put the "template" keyword after the access token. Great, thanks! CONTEXT In C++, when referencing a class member (using ., -> or ::) in a dependent context, if that member is a template, the template keyword is required after the access token. For example: template void MyDependentTemplate(T t) { t.DoSomething(); /* Error - t is dependent. "<" is treated as a less-than sign because DoSomething is assumed to be a non-template identifier. */ t.template DoSomething(); // Good. } PATCH EXPLANATION In order to throw an appropriate error message we need to identify and/or create a point in the compiler where the following conditions are all simultaneously satisfied: A) a class member access token (., ->, ::) B) a dependent context C) the thing being accessed is a template-id D) No template keyword A, B and D are relatively straightforward and I won't go into detail about how that was achieved. The real challenge is C - figuring out whether a name is a template-id. Usually, we would look up the name and use that to determine whether the name is a template or not. But, we cannot do that. We are in a dependent context, so lookup cannot (in theory) find anything at the point the access expression is parsed. We (maybe) could defer checking until the template is actually instantiated. But this is not the approach I have gone for, since this to me sounded unnecessarily awkward and clunky. In my mind this also seems like a syntax error, and IMO it seems more natural that syntax errors should get caught as soon as they are parsed. So, the solution I went for was to figure out whether the name was a template by looking at the surrounding tokens. The key to this lies in being able to differentiate between the start and end of a template parameter list (< and >) and greater-than and less-than tokens (and perhaps also things like << or >>). If the final > (if we find one at all) does indeed mark the end of a class or function template, then it will be followed by something like (, ::, or even just ;. On the other hand, a greater-than operator would be followed by a name. PERFORMANCE IMPACT My concern with this approach was that checking this would actually slow the compiler down. In the end it seemed the opposite was true. By parsing the tokens to determine whether the name looks like a template-id, we can cut out a lot of the template-checking code that gets run trying to find a template when there is none, making compile time generally faster. That being said, I'm not sure if the improvement will be that substantial, so I did not feel it necessary to introduce the template checking method everywhere where we could possibly have run into a template. I ran an ABAB test with the following code repeated enough times to fill up about 10,000 lines: ai = 1; bi = 2; ci = 3; c.x = 4; (&c)->x = 5; mytemplateclass::y = 6; c.template class_func(); (&c)->template class_func(); mytemplateclass::template class_func_static(); c2.class_func(); (&c2)->class_func(); myclass::class_func_static(); ai = 6; bi = 5; ci = 4; c.x = 3; (&c)->x = 2; mytemplateclass::y = 1; c.template class_func(); (&c)->template class_func(); mytemplateclass::template class_func_static(); c2.class_func(); (&c2)->class_func(); myclass::class_func_static(); It basically just contains a mixture of class access expressions plus some regular assignments for good measure. The compiler was compiled without any optimisations (and so slower than usual). In hindsight, perhaps this test case assumes the worst-ish-case scenario since it contains a lot of templates; most of the benefits of this patch occur when parsing non-templates. The compiler (clean) and the compiler with the patch applied (patched) compiled the above code 20 times each in ABAB fashion. On average, the clean compiler took 2.26295 seconds and the patched compiler took 2.25145 seconds (about 0.508% faster). Whether this improvement remains or disappears when the compiler is built with optimisations turned on I haven't tested. My main concern was just making sure it didn't get any slower. AWKWARD TEST CASES You will see from the patch that it required the updating of a few testcases (as well as one place within the compiler). I believe this is because up until now, the compiler allowed the omission of the template keyword in some places it shouldn't have. Take decltype34.C for example: struct impl { template static T create(); }; template()->impl::create())> struct tester{}; GCC currently accepts this
Re: [PATCH] c++: error message for dependent template members [PR70417]
Re-adding gcc-patches@gcc.gnu.org. -- Forwarded message - From: Anthony Sharp Date: Fri, 17 Sept 2021 at 23:11 Subject: Re: [PATCH] c++: error message for dependent template members [PR70417] To: Jason Merrill Hi Jason! Apologies for the delay. > This is basically core issue 1835, http://wg21.link/cwg1835 > This was changed for C++23 by the paper "Declarations and where to find > them", http://wg21.link/p1787 Interesting, I was not aware of that. I was very vaguely aware that a template-id in a class member access expression could be found by ordinary lookup (very bottom of here https://en.cppreference.com/w/cpp/language/dependent_name), but it's interesting to see it is deeper than I realised. > But in either case, whether create is in a dependent scope depends on > how we resolve impl::, we don't need to remember further back in the > expression. So your dependent_expression_p parameter seems like the > wrong approach. Note that when we're looking up the name after ->, the > type of the object expression is in parser->context->object_type. That's true. I think my thinking was that since it already got figured out in cp_parser_postfix_dot_deref_expression, which is where . and -> access expressions come from, I thought I might as well pass it through, since it seemed to work. But looking again, you're right, it's not really worth the hassle; might as well just call dependent_scope_p again. > The cases you fixed in symbol-summary.h are indeed mistakes, but not > ill-formed, so giving an error on them is wrong. For example, here is a > well-formed program that is rejected with your patch: > template void f(T t) { t.m(0); } > struct A { int m; } a; > int main() { f(a); } I suppose there was always going to be edge-cases when doing it the way I've done. But yes, it can be worked-around by making it a warning instead. Interestingly Clang doesn't trip up on that example, so I guess they must be examining it some other way (e.g. at instantiation time) - but that approach perhaps misses out on the slight performance improvement this seems to bring. > Now that we're writing C++, I'd prefer to avoid this kind of pattern in > favor of RAII, such as saved_token_sentinel. If this is still relevant > after addressing the above comments. Sorry, it's the junior developer in me showing! So this confused me at first. After having mucked around a bit I tried using saved_token_sentinel but didn't see any benefit since it doesn't rollback on going out of scope, and I'll always want to rollback. I can call rollback directly, but then I might as well save and restore myself. So what I did was use it but also modify it slightly to rollback by default on going out of scope (in my mind that makes more sense, since if something goes wrong you wouldn't normally want to commit anything that happened [edit 1: unless committing was part of the whole sanity checking thing] [edit 2: well I guess you could also argue that since this is a parser afterall, we like to KEEP things sometimes]). But anyways, I made this configurable; it now has three modes - roll-back, commit or do nothing. Let me know if you think that's not the way to go. > This code doesn't handle skipping matched ()/{}/[] in the > template-argument-list. You probably want to involve > cp_parser_skip_to_end_of_template_parameter_list somehow. Good point. It required some refactoring, but I have used it. Also, just putting it out there, this line from cp_parser_skip_to_end_of_template_parameter_list makes zero sense to me (why throw an error OR immediately return?), but I have worked around it, since it seems to break without it: > /* Are we ready, yet? If not, issue error message. */ > if (cp_parser_require (parser, CPP_GREATER, RT_GREATER)) > return false; Last thing - I initially made a mistake. I put something like: (next_token->type == CPP_NAME && MAYBE_CLASS_TYPE_P (parser->scope) && !constructor_name_p (cp_expr (next_token->u.value, next_token->location), parser->scope)) Instead of: !(next_token->type == CPP_NAME && MAYBE_CLASS_TYPE_P (parser->scope) && constructor_name_p (cp_expr (next_token->u.value, next_token->location), parser->scope)) This meant a lot of things were being excluded that weren't supposed to be. Oops! Changing this opened up a whole new can of worms, so I had to make some changes to the main logic, but it just a little bit in the end. Regtested everything again and all seems fine. Bootstraps fine. Patch attached. Let me know if it needs anything else. Tha
Re: [PATCH] c++: error message for dependent template members [PR70417]
And also re-attaching the patch! On Fri, 17 Sept 2021 at 23:17, Anthony Sharp wrote: > > Re-adding gcc-patches@gcc.gnu.org. > > -- Forwarded message - > From: Anthony Sharp > Date: Fri, 17 Sept 2021 at 23:11 > Subject: Re: [PATCH] c++: error message for dependent template members > [PR70417] > To: Jason Merrill > > > Hi Jason! Apologies for the delay. > > > This is basically core issue 1835, http://wg21.link/cwg1835 > > > This was changed for C++23 by the paper "Declarations and where to find > > them", http://wg21.link/p1787 > > Interesting, I was not aware of that. I was very vaguely aware that a > template-id in a class member access expression could be found by > ordinary lookup (very bottom of here > https://en.cppreference.com/w/cpp/language/dependent_name), but it's > interesting to see it is deeper than I realised. > > > But in either case, whether create is in a dependent scope depends on > > how we resolve impl::, we don't need to remember further back in the > > expression. So your dependent_expression_p parameter seems like the > > wrong approach. Note that when we're looking up the name after ->, the > > type of the object expression is in parser->context->object_type. > > That's true. I think my thinking was that since it already got figured > out in cp_parser_postfix_dot_deref_expression, which is where . and -> > access expressions come from, I thought I might as well pass it > through, since it seemed to work. But looking again, you're right, > it's not really worth the hassle; might as well just call > dependent_scope_p again. > > > The cases you fixed in symbol-summary.h are indeed mistakes, but not > > ill-formed, so giving an error on them is wrong. For example, here is a > > well-formed program that is rejected with your patch: > > > template void f(T t) { t.m(0); } > > struct A { int m; } a; > > int main() { f(a); } > > I suppose there was always going to be edge-cases when doing it the > way I've done. But yes, it can be worked-around by making it a warning > instead. Interestingly Clang doesn't trip up on that example, so I > guess they must be examining it some other way (e.g. at instantiation > time) - but that approach perhaps misses out on the slight performance > improvement this seems to bring. > > > Now that we're writing C++, I'd prefer to avoid this kind of pattern in > > favor of RAII, such as saved_token_sentinel. If this is still relevant > > after addressing the above comments. > > Sorry, it's the junior developer in me showing! So this confused me at > first. After having mucked around a bit I tried using > saved_token_sentinel but didn't see any benefit since it doesn't > rollback on going out of scope, and I'll always want to rollback. I > can call rollback directly, but then I might as well save and restore > myself. So what I did was use it but also modify it slightly to > rollback by default on going out of scope (in my mind that makes more > sense, since if something goes wrong you wouldn't normally want to > commit anything that happened [edit 1: unless committing was part of > the whole sanity checking thing] [edit 2: well I guess you could also > argue that since this is a parser afterall, we like to KEEP things > sometimes]). But anyways, I made this configurable; it now has three > modes - roll-back, commit or do nothing. Let me know if you think > that's not the way to go. > > > This code doesn't handle skipping matched ()/{}/[] in the > > template-argument-list. You probably want to involve > > cp_parser_skip_to_end_of_template_parameter_list somehow. > > Good point. It required some refactoring, but I have used it. Also, > just putting it out there, this line from > cp_parser_skip_to_end_of_template_parameter_list makes zero sense to > me (why throw an error OR immediately return?), but I have worked > around it, since it seems to break without it: > > > /* Are we ready, yet? If not, issue error message. */ > > if (cp_parser_require (parser, CPP_GREATER, RT_GREATER)) > > return false; > > Last thing - I initially made a mistake. I put something like: > > (next_token->type == CPP_NAME > && MAYBE_CLASS_TYPE_P (parser->scope) > && !constructor_name_p (cp_expr (next_token->u.value, > > next_token->location), >parser->scope)) > > Instead of: > > !(next_token->type == CPP_NAME > && MAYBE_CLASS_TYPE_P (parser->scope) &
Re: [PATCH] c++: error message for dependent template members [PR70417]
On 9/17/21 18:22, Anthony Sharp wrote: And also re-attaching the patch! On Fri, 17 Sept 2021 at 23:17, Anthony Sharp wrote: Re-adding gcc-patches@gcc.gnu.org. -- Forwarded message - From: Anthony Sharp Date: Fri, 17 Sept 2021 at 23:11 Subject: Re: [PATCH] c++: error message for dependent template members [PR70417] To: Jason Merrill Hi Jason! Apologies for the delay. This is basically core issue 1835, http://wg21.link/cwg1835 This was changed for C++23 by the paper "Declarations and where to find them", http://wg21.link/p1787 Interesting, I was not aware of that. I was very vaguely aware that a template-id in a class member access expression could be found by ordinary lookup (very bottom of here https://en.cppreference.com/w/cpp/language/dependent_name), but it's interesting to see it is deeper than I realised. But in either case, whether create is in a dependent scope depends on how we resolve impl::, we don't need to remember further back in the expression. So your dependent_expression_p parameter seems like the wrong approach. Note that when we're looking up the name after ->, the type of the object expression is in parser->context->object_type. That's true. I think my thinking was that since it already got figured out in cp_parser_postfix_dot_deref_expression, which is where . and -> access expressions come from, I thought I might as well pass it through, since it seemed to work. But looking again, you're right, it's not really worth the hassle; might as well just call dependent_scope_p again. The cases you fixed in symbol-summary.h are indeed mistakes, but not ill-formed, so giving an error on them is wrong. For example, here is a well-formed program that is rejected with your patch: template void f(T t) { t.m(0); } struct A { int m; } a; int main() { f(a); } I suppose there was always going to be edge-cases when doing it the way I've done. But yes, it can be worked-around by making it a warning instead. Interestingly Clang doesn't trip up on that example, so I guess they must be examining it some other way (e.g. at instantiation time) - but that approach perhaps misses out on the slight performance improvement this seems to bring. Now that we're writing C++, I'd prefer to avoid this kind of pattern in favor of RAII, such as saved_token_sentinel. If this is still relevant after addressing the above comments. Sorry, it's the junior developer in me showing! So this confused me at first. After having mucked around a bit I tried using saved_token_sentinel but didn't see any benefit since it doesn't rollback on going out of scope, and I'll always want to rollback. I can call rollback directly, but then I might as well save and restore myself. So what I did was use it but also modify it slightly to rollback by default on going out of scope (in my mind that makes more sense, since if something goes wrong you wouldn't normally want to commit anything that happened [edit 1: unless committing was part of the whole sanity checking thing] [edit 2: well I guess you could also argue that since this is a parser after all, we like to KEEP things sometimes]). But anyways, I made this configurable; it now has three modes - roll-back, commit or do nothing. Let me know if you think that's not the way to go. I like adding the configurability, but I think let's keep committing as the default behavior. And adding the parameter to the rollback function seems unnecessary. For the behavior argument, let's use an enum to be clearer. This code doesn't handle skipping matched ()/{}/[] in the template-argument-list. You probably want to involve cp_parser_skip_to_end_of_template_parameter_list somehow. Good point. It required some refactoring, but I have used it. Also, just putting it out there, this line from cp_parser_skip_to_end_of_template_parameter_list makes zero sense to me (why throw an error OR immediately return?), but I have worked around it, since it seems to break without it: /* Are we ready, yet? If not, issue error message. */ if (cp_parser_require (parser, CPP_GREATER, RT_GREATER)) return false; If the next token is >, we're already at the end of the template argument list. If not, then something has gone wrong, so give an error and skip ahead. Last thing - I initially made a mistake. I put something like: (next_token->type == CPP_NAME && MAYBE_CLASS_TYPE_P (parser->scope) && !constructor_name_p (cp_expr (next_token->u.value, next_token->location), parser->scope)) Instead of: !(next_token->type == CPP_NAME && MAYBE_CLASS_TYPE_P (parser->scope) && constructor_name_p (cp_expr (next_token->u.value,
Re: [PATCH] c++: error message for dependent template members [PR70417]
in cp_parser_id_expression in that case. The reason it throws a warning is because typename14.C is intentionally malformed; in the eyes of the compiler it's written like an expression because it's missing the return type (although, even adding a return type would not make it valid). I'm not sure there's any worthwhile way around this really. You could tell DejaGnu to silence the warning, but I think it's probably more informative to just include the warning inline. Also, some more missing template keywords seemed to crop up in the regression tests, so I had to sprinkle some more template keywords in a few. I guess there was a hidden bug that was missing a few scenarios. Just out of curiosity, how pedantic would you say I should be when submitting patches to GCC etc? I regression test and bootstrap all my changes, but I'm always worrying about what might happen if I somehow forgot to stage a file in git, or attached the wrong patch file, or interpreted the .sum file wrong etc. Do patches go through another round of testing after submitting? Sometimes I wonder whether I should be applying the patch locally and then bootstrapping and regression testing it again, although that's hardly fun since that usually takes around 2-3 hours even with -j 12. Maybe I ought to think about getting a dedicated Linux PC. Patch should be attached. Kind regards, Anthony On Wed, 22 Sept 2021 at 21:04, Jason Merrill wrote: > On 9/17/21 18:22, Anthony Sharp wrote: > > And also re-attaching the patch! > > > > On Fri, 17 Sept 2021 at 23:17, Anthony Sharp > wrote: > >> > >> Re-adding gcc-patches@gcc.gnu.org. > >> > >> -- Forwarded message - > >> From: Anthony Sharp > >> Date: Fri, 17 Sept 2021 at 23:11 > >> Subject: Re: [PATCH] c++: error message for dependent template members > [PR70417] > >> To: Jason Merrill > >> > >> > >> Hi Jason! Apologies for the delay. > >> > >>> This is basically core issue 1835, http://wg21.link/cwg1835 > >> > >>> This was changed for C++23 by the paper "Declarations and where to find > >>> them", http://wg21.link/p1787 > >> > >> Interesting, I was not aware of that. I was very vaguely aware that a > >> template-id in a class member access expression could be found by > >> ordinary lookup (very bottom of here > >> https://en.cppreference.com/w/cpp/language/dependent_name), but it's > >> interesting to see it is deeper than I realised. > >> > >>> But in either case, whether create is in a dependent scope depends > on > >>> how we resolve impl::, we don't need to remember further back in the > >>> expression. So your dependent_expression_p parameter seems like the > >>> wrong approach. Note that when we're looking up the name after ->, the > >>> type of the object expression is in parser->context->object_type. > >> > >> That's true. I think my thinking was that since it already got figured > >> out in cp_parser_postfix_dot_deref_expression, which is where . and -> > >> access expressions come from, I thought I might as well pass it > >> through, since it seemed to work. But looking again, you're right, > >> it's not really worth the hassle; might as well just call > >> dependent_scope_p again. > >> > >>> The cases you fixed in symbol-summary.h are indeed mistakes, but not > >>> ill-formed, so giving an error on them is wrong. For example, here is > a > >>> well-formed program that is rejected with your patch: > >> > >>> template void f(T t) { t.m(0); } > >>> struct A { int m; } a; > >>> int main() { f(a); } > >> > >> I suppose there was always going to be edge-cases when doing it the > >> way I've done. But yes, it can be worked-around by making it a warning > >> instead. Interestingly Clang doesn't trip up on that example, so I > >> guess they must be examining it some other way (e.g. at instantiation > >> time) - but that approach perhaps misses out on the slight performance > >> improvement this seems to bring. > >> > >>> Now that we're writing C++, I'd prefer to avoid this kind of pattern in > >>> favor of RAII, such as saved_token_sentinel. If this is still relevant > >>> after addressing the above comments. > >> > >> Sorry, it's the junior developer in me showing! So this confused me at > >> first. After having mucked around a bit I tried using > >> saved_token_se
Re: [PATCH] c++: error message for dependent template members [PR70417]
On 10/10/21 07:28, Anthony Sharp wrote: Hi Jason, Hope you are well. Thanks for the feedback. I like adding the configurability, but I think let's keep committing as the default behavior. And adding the parameter to the rollback function seems unnecessary. For the behavior argument, let's use an enum to be clearer. Sure, all done. The declaration of the enum could have gone in many places it seems but I put it in cp-tree.h. I'd put it just above the definition of saved_token_sentinel in parser.c. I've removed the awkward call to cp_parser_require and put it (along with the call to cp_parser_skip_to_end_of_template_parameter_list) in its own new function called cp_parser_ensure_reached_end_of_template_parameter_list. Maybe cp_parser_require_end_of_template_parameter_list? Either way is fine. I think we don't want to return early here, we want to go through the same ( check that we do for regular names. I have changed it to do that, but could something like "operator- <" ever be intended as something other than the start of a template-id? Hmm, good point; operators that are member functions must be non-static, so we couldn't be doing a comparison of the address of the function. +// { dg-warning "expected \"template\" keyword before dependent template name" { target c++20 } .-1 } +// ^ bogus warning caused by the above being treated as an expression, not a declaration > Hmm, that's a problem. Can you avoid it by checking declarator_p? We actually already check declarator_p in cp_parser_id_expression in that case. The reason it throws a warning is because typename14.C is intentionally malformed; in the eyes of the compiler it's written like an expression because it's missing the return type (although, even adding a return type would not make it valid). I'm not sure there's any worthwhile way around this really. But it isn't written like an expression: the error comes when trying to diagnose it as an invalid type in a declaration context. So declarator_p should be true there. I'll fix that. Also, some more missing template keywords seemed to crop up in the regression tests, so I had to sprinkle some more template keywords in a few. I guess there was a hidden bug that was missing a few scenarios. Just out of curiosity, how pedantic would you say I should be when submitting patches to GCC etc? I regression test and bootstrap all my changes, but I'm always worrying about what might happen if I somehow forgot to stage a file in git, or attached the wrong patch file, or interpreted the .sum file wrong etc. Do patches go through another round of testing after submitting? Not automatically (yet), but often I test other people's patches before applying them. Sometimes I wonder whether I should be applying the patch locally and then bootstrapping and regression testing it again, although that's hardly fun since that usually takes around 2-3 hours even with -j 12. Maybe I ought to think about getting a dedicated Linux PC. You could also apply for a GCC Compile Farm account: https://gcc.gnu.org/wiki/CompileFarm + if (next_token->keyword == RID_TEMPLATE) +{ + /* But at least make sure it's properly formed (e.g. see PR19397). */ + if (cp_lexer_peek_nth_token (parser->lexer, 2)->type == CPP_NAME) + return 1; + + return -1; +} + + /* Could be a ~ referencing the destructor of a class template. */ + if (next_token->type == CPP_COMPL) +{ + /* It could only be a template. */ + if (cp_lexer_peek_nth_token (parser->lexer, 2)->type == CPP_NAME) + return 1; + + return -1; +} Why don't these check for the < ? + /* E.g. "::operator- <>". */ + if (next_token->keyword == RID_OPERATOR) +{ + /* Consume it so it doesn't get in our way. */ + cp_lexer_consume_token (parser->lexer); + next_token = cp_lexer_peek_token (parser->lexer); + found_operator_keyword = true; +} ... + if (!found_operator_keyword && next_token->type != CPP_NAME) +return -1; These could be if/else if so you don't need the found_operator_keyword variable. + if (next_token->type == CPP_TEMPLATE_ID) +return 1; This could move above the saved_token_sentinel; you won't have a CPP_TEMPLATE_ID after RID_OPERATOR. + /* If this is a function template then we would see a "(" after the + final ">". It could also be a class template constructor. */ + if (next_token->type == CPP_OPEN_PAREN + /* If this is a class template then we could see a scope token after + the final ">". */ + || next_token->type == CPP_SCOPE + /* We could see a ";" after a variable template. */ + || next_token->type == CPP_SEMICOLON + /* We could see something like +friend vect (::operator- <>)( const vect&, const vect& ); +*/ + || next_token->type == CPP_CLOSE_PAREN) +return 1; This seems too limited. As I was saying before,
Re: [PATCH] c++: error message for dependent template members [PR70417]
Hi Jason, Hope you are well. Apologies for not coming back sooner. >I'd put it just above the definition of saved_token_sentinel in parser.c. Sounds good, done. >Maybe cp_parser_require_end_of_template_parameter_list? Either way is fine. Even better, have changed it. >Hmm, good point; operators that are member functions must be non-static, >so we couldn't be doing a comparison of the address of the function. In that case I have set it to return early there. >So declarator_p should be true there. I'll fix that. Thank you. >> + if (next_token->keyword == RID_TEMPLATE) >> +{ >> + /* But at least make sure it's properly formed (e.g. see PR19397). */ >> + if (cp_lexer_peek_nth_token (parser->lexer, 2)->type == CPP_NAME) >> + return 1; >> + >> + return -1; >> +} >> + >> + /* Could be a ~ referencing the destructor of a class template. */ >> + if (next_token->type == CPP_COMPL) >> +{ >> + /* It could only be a template. */ >> + if (cp_lexer_peek_nth_token (parser->lexer, 2)->type == CPP_NAME) >> + return 1; >> + >> + return -1; >> +} > >Why don't these check for the < ? I think perhaps I could have named the function better; instead of next_token_begins_template_id, how's about next_token_begins_template_name? That's all I intended to check for. In the first case, something like "->template some_name" will always be intended as a template, so no need to check for the <. If there were default template arguments you could also validly omit the <> completely, I think (could be wrong). In the second case I think you are correct. I have added a check for the "<". >> + /* E.g. "::operator- <>". */ >> + if (next_token->keyword == RID_OPERATOR) >> +{ >> + /* Consume it so it doesn't get in our way. */ >> + cp_lexer_consume_token (parser->lexer); >> + next_token = cp_lexer_peek_token (parser->lexer); >> + found_operator_keyword = true; >> +} >> + >> + if (next_token->type == CPP_TEMPLATE_ID) >> + return 1; >> + >> + /* By now the next token should be a name/operator and the one after that >> + should be a "<". */ >> + if (!cp_parser_nth_token_starts_template_argument_list_p (parser, 2)) >> +return -1; >> + >> + if (!found_operator_keyword && next_token->type != CPP_NAME) >> +return -1; > >These could be if/else if so you don't need the found_operator_keyword >variable. Hmm yes I think so. But that's been refactored now anyways; it returns if it sees RID_OPERATOR. >> + if (next_token->type == CPP_TEMPLATE_ID) >> +return 1; > >This could move above the saved_token_sentinel; you won't have a >CPP_TEMPLATE_ID after RID_OPERATOR. Yes thanks, done. >> + /* If this is a function template then we would see a "(" after the >> + final ">". It could also be a class template constructor. */ >> + if (next_token->type == CPP_OPEN_PAREN >> + /* If this is a class template then we could see a scope token after >> + the final ">". */ >> + || next_token->type == CPP_SCOPE >> + /* We could see a ";" after a variable template. */ >> + || next_token->type == CPP_SEMICOLON >> + /* We could see something like >> +friend vect (::operator- <>)( const vect&, const vect& ); >> +*/ >> + || next_token->type == CPP_CLOSE_PAREN) >> +return 1; > >This seems too limited. As I was saying before, > >> I guess any operator-or-punctuator after the > would suggest a >> template-id. > >For instance, the patch doesn't currently warn about > >template struct A >{ > template static U m; >}; > >template int f (T t) >{ > return t.m + 42; >} > >This is especially a problem since we use the return value of -1 to skip >calling cp_parser_template_id_expr. Maybe a better approach then as you hinted at before would be to check for what we would not expect to see (i.e. a name), rather than what we would. I've updated it. Seems to work? >> +++ b/gcc/testsuite/g++.dg/cpp0x/decltype34.C >> @@ -7,13 +7,13 @@ struct impl >> }; >> >> template> -class = decltype(impl::create()->impl::create())> >> +class = decltype(impl::create()->impl::template create())> > >As I was saying in earlier discussion, this is currently a false >positive, because impl:: is a non-dependent scope. If parser->scope is >set, we don't need to look at parser->context->object_type... > >> + && (dependent_scope_p (parser->context->object_type) >> + /* :: access expressions. */ >> + || (dependent_scope_p (parser->scope) > >...here. Ah yes I see now. Sorry, I misunderstood what you had said before. I thought you were saying that there is no need to check whether the entire expression is dependent, merely the class we are trying to access (although my hunch is that if any part of an expression is dependent, the whole thing must be). But yes that regression test confused me quite a bit, having read the issue that it was trying to solve it makes a lot
Re: [PATCH] c++: error message for dependent template members [PR70417]
On 12/4/21 12:23, Anthony Sharp wrote: Hi Jason, Hope you are well. Apologies for not coming back sooner. >I'd put it just above the definition of saved_token_sentinel in parser.c. Sounds good, done. >Maybe cp_parser_require_end_of_template_parameter_list? Either way is fine. Even better, have changed it. >Hmm, good point; operators that are member functions must be non-static, >so we couldn't be doing a comparison of the address of the function. In that case I have set it to return early there. >So declarator_p should be true there. I'll fix that. Thank you. >> + if (next_token->keyword == RID_TEMPLATE) >> + { >> + /* But at least make sure it's properly formed (e.g. see PR19397). */ >> + if (cp_lexer_peek_nth_token (parser->lexer, 2)->type == CPP_NAME) >> + return 1; >> + >> + return -1; >> + } >> + >> + /* Could be a ~ referencing the destructor of a class template. */ >> + if (next_token->type == CPP_COMPL) >> + { >> + /* It could only be a template. */ >> + if (cp_lexer_peek_nth_token (parser->lexer, 2)->type == CPP_NAME) >> + return 1; >> + >> + return -1; >> + } > >Why don't these check for the < ? I think perhaps I could have named the function better; instead of next_token_begins_template_id, how's about next_token_begins_template_name? That's all I intended to check for. You're using it to control whether we try to parse a template-id, and it's used to initialize variables named looks_like_template_id, so I think better to keep the old name. In the first case, something like "->template some_name" will always be intended as a template, so no need to check for the <. If there were default template arguments you could also validly omit the <> completely, I think (could be wrong). Or if the template arguments can be deduced, yes: template struct A { template void f(U u); }; template void g(A a) { a->template f(42); } But 'f' is still not a template-id. ... Actually, it occurs to me that you might be better off handling this in cp_parser_template_name, something like the below, to avoid the complex duplicate logic in the id-expression handling. Note that in this patch I'm using "any_object_scope" as a proxy for "I'm parsing an expression", since !is_declaration doesn't work for that; as a result, this doesn't handle the static member function template case. For that you'd probably still need to pass down a flag so that cp_parser_template_name knows it's being called from cp_parser_id_expression. Your patch has a false positive on template struct A { }; template void f() { A(); }; which my patch checks in_template_argument_list_p to avoid, though checking any_object_scope also currently avoids it. What do you think? JasonFrom 3022a600514f8adf8706fd286387a5f9f34c06ba Mon Sep 17 00:00:00 2001 From: Jason Merrill Date: Wed, 8 Dec 2021 17:24:21 -0500 Subject: [PATCH] handle in cp_parser_template_name To: gcc-patches@gcc.gnu.org --- gcc/cp/parser.c | 41 - 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index ed0bae33fc1..6ec9da646cf 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -18546,6 +18546,8 @@ cp_parser_template_name (cp_parser* parser, /* cp_parser_lookup_name clears OBJECT_TYPE. */ tree scope = (parser->scope ? parser->scope : parser->context->object_type); + bool any_object_scope = (parser->context->object_type + || parser->object_scope); /* Look up the name. */ decl = cp_parser_lookup_name (parser, identifier, @@ -18625,12 +18627,41 @@ cp_parser_template_name (cp_parser* parser, found = true; } - /* "in a type-only context" */ if (!found && scope - && tag_type != none_type - && dependentish_scope_p (scope) - && cp_parser_nth_token_starts_template_argument_list_p (parser, 1)) - found = true; + && processing_template_decl + && cp_parser_nth_token_starts_template_argument_list_p (parser, 1) + && dependentish_scope_p (scope)) + { + /* "in a type-only context" */ + if (tag_type != none_type) + found = true; + else if (warn_missing_template_keyword + /* If there's an object scope we know we're in an + expression, not a declarator-id. FIXME is_declaration + ought to make that distinction for us. */ + && any_object_scope + /* In a template argument list a > could be closing + the enclosing targs. */ + && !parser->in_template_argument_list_p + && !token->error_reported + && warning_enabled_at (token->location, + OPT_Wmissing_template_keyword)) + { + saved_token_sentinel toks (parser->lexer, STS_ROLLBACK); + if (cp_parser_skip_entire_template_parameter_list (parser) + /* An operator after the > suggests that the > ends a + template-id; a name or literal suggests that the > is an + operator. */ + && (cp_lexer_p
Re: [PATCH] c++: error message for dependent template members [PR70417]
On 12/9/21 10:51, Jason Merrill wrote: On 12/4/21 12:23, Anthony Sharp wrote: Hi Jason, Hope you are well. Apologies for not coming back sooner. >I'd put it just above the definition of saved_token_sentinel in parser.c. Sounds good, done. >Maybe cp_parser_require_end_of_template_parameter_list? Either way is fine. Even better, have changed it. >Hmm, good point; operators that are member functions must be non-static, >so we couldn't be doing a comparison of the address of the function. In that case I have set it to return early there. >So declarator_p should be true there. I'll fix that. Thank you. >> + if (next_token->keyword == RID_TEMPLATE) >> + { >> + /* But at least make sure it's properly formed (e.g. see PR19397). */ >> + if (cp_lexer_peek_nth_token (parser->lexer, 2)->type == CPP_NAME) >> + return 1; >> + >> + return -1; >> + } >> + >> + /* Could be a ~ referencing the destructor of a class template. */ >> + if (next_token->type == CPP_COMPL) >> + { >> + /* It could only be a template. */ >> + if (cp_lexer_peek_nth_token (parser->lexer, 2)->type == CPP_NAME) >> + return 1; >> + >> + return -1; >> + } > >Why don't these check for the < ? I think perhaps I could have named the function better; instead of next_token_begins_template_id, how's about next_token_begins_template_name? That's all I intended to check for. You're using it to control whether we try to parse a template-id, and it's used to initialize variables named looks_like_template_id, so I think better to keep the old name. In the first case, something like "->template some_name" will always be intended as a template, so no need to check for the <. If there were default template arguments you could also validly omit the <> completely, I think (could be wrong). Or if the template arguments can be deduced, yes: template struct A { template void f(U u); }; template void g(A a) { a->template f(42); } But 'f' is still not a template-id. ... Actually, it occurs to me that you might be better off handling this in cp_parser_template_name, something like the below, to avoid the complex duplicate logic in the id-expression handling. Note that in this patch I'm using "any_object_scope" as a proxy for "I'm parsing an expression", since !is_declaration doesn't work for that; as a result, this doesn't handle the static member function template case. For that you'd probably still need to pass down a flag so that cp_parser_template_name knows it's being called from cp_parser_id_expression. Your patch has a false positive on template struct A { }; template void f() { A(); }; which my patch checks in_template_argument_list_p to avoid, though checking any_object_scope also currently avoids it. What do you think? I decided that it made more sense to keep the check in cp_parser_id_expression like you had it, but I moved it to the end to simplify the logic. Here's what I'm applying, thanks!From 1978f05716133b934de0fca7c3d64089b62e3e78 Mon Sep 17 00:00:00 2001 From: Anthony Sharp Date: Sat, 4 Dec 2021 17:23:22 + Subject: [PATCH] c++: warning for dependent template members [PR70417] To: gcc-patches@gcc.gnu.org Add a helpful warning message for when the user forgets to include the "template" keyword after ., -> or :: when accessing a member in a dependent context, where the member is a template. PR c++/70417 gcc/c-family/ChangeLog: * c.opt: Added -Wmissing-template-keyword. gcc/cp/ChangeLog: * parser.c (cp_parser_id_expression): Handle -Wmissing-template-keyword. (struct saved_token_sentinel): Add modes to control what happens on destruction. (cp_parser_statement): Adjust. (cp_parser_skip_entire_template_parameter_list): New function that skips an entire template parameter list. (cp_parser_require_end_of_template_parameter_list): Rename old cp_parser_skip_to_end_of_template_parameter_list. (cp_parser_skip_to_end_of_template_parameter_list): Refactor to be called from one of the above two functions. (cp_parser_lambda_declarator_opt) (cp_parser_explicit_template_declaration) (cp_parser_enclosed_template_argument_list): Adjust. gcc/ChangeLog: * doc/invoke.texi: Documentation for Wmissing-template-keyword. gcc/testsuite/ChangeLog: * g++.dg/cpp0x/variadic-mem_fn2.C: Catch warning about missing template keyword. * g++.dg/template/dependent-name17.C: New test. * g++.dg/template/dependent-name18.C: New test. Co-authored-by: Jason Merrill --- gcc/doc/invoke.texi | 33 gcc/c-family/c.opt| 4 + gcc/cp/parser.c | 178 +- gcc/testsuite/g++.dg/cpp0x/variadic-mem_fn2.C | 1 + .../g++.dg/template/dependent-name17.C| 49 + .../g++.dg/template/dependent-name18.C| 5 + 6 files changed, 223 insertions(+), 47 deletions(-) create mode 100644 gcc/testsui
Re: [PATCH] c++: error message for dependent template members [PR70417]
Hi Jason, Hope you are well. Apologies, I've not had time to sit down and look at this since last month I quit my old job, then I had family around for the whole of the Christmas period, and then even more recently I've had to start my new job. In any case happy that you managed to figure it all out. I noticed the small regression at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104025. To be honest I wouldn't even know how to begin to go about fixing that so perhaps it's best if I leave that to you? I guess it's not playing well with the use of warn_missing_template_keyword. Maybe I didn't set that variable up properly or something. Kind regards, Anthony On Thu, 13 Jan 2022 at 21:01, Jason Merrill wrote: > On 12/9/21 10:51, Jason Merrill wrote: > > On 12/4/21 12:23, Anthony Sharp wrote: > >> Hi Jason, > >> > >> Hope you are well. Apologies for not coming back sooner. > >> > >> >I'd put it just above the definition of saved_token_sentinel in > >> parser.c. > >> > >> Sounds good, done. > >> > >> >Maybe cp_parser_require_end_of_template_parameter_list? Either way > >> is fine. > >> > >> Even better, have changed it. > >> > >> >Hmm, good point; operators that are member functions must be > >> non-static, > >> >so we couldn't be doing a comparison of the address of the function. > >> > >> In that case I have set it to return early there. > >> > >> >So declarator_p should be true there. I'll fix that. > >> > >> Thank you. > >> > >> >> + if (next_token->keyword == RID_TEMPLATE) > >> >> +{ > >> >> + /* But at least make sure it's properly formed (e.g. see > >> PR19397). */ > >> >> + if (cp_lexer_peek_nth_token (parser->lexer, 2)->type == > >> CPP_NAME) > >> >> + return 1; > >> >> + > >> >> + return -1; > >> >> +} > >> >> + > >> >> + /* Could be a ~ referencing the destructor of a class template. > >> */ > >> >> + if (next_token->type == CPP_COMPL) > >> >> +{ > >> >> + /* It could only be a template. */ > >> >> + if (cp_lexer_peek_nth_token (parser->lexer, 2)->type == > >> CPP_NAME) > >> >> + return 1; > >> >> + > >> >> + return -1; > >> >> +} > >> > > >> >Why don't these check for the < ? > >> > >> I think perhaps I could have named the function better; instead of > >> next_token_begins_template_id, how's about > >> next_token_begins_template_name? > >> That's all I intended to check for. > > > > You're using it to control whether we try to parse a template-id, and > > it's used to initialize variables named looks_like_template_id, so I > > think better to keep the old name. > > > >> In the first case, something like "->template some_name" will always be > >> intended as a template, so no need to check for the <. If there were > >> default > >> template arguments you could also validly omit the <> completely, I > think > >> (could be wrong). > > > > Or if the template arguments can be deduced, yes: > > > > template struct A > > { > >template void f(U u); > > }; > > > > template void g(A a) > > { > >a->template f(42); > > } > > > > But 'f' is still not a template-id. > > > > ... > > > > Actually, it occurs to me that you might be better off handling this in > > cp_parser_template_name, something like the below, to avoid the complex > > duplicate logic in the id-expression handling. > > > > Note that in this patch I'm using "any_object_scope" as a proxy for "I'm > > parsing an expression", since !is_declaration doesn't work for that; as > > a result, this doesn't handle the static member function template case. > > For that you'd probably still need to pass down a flag so that > > cp_parser_template_name knows it's being called from > > cp_parser_id_expression. > > > > Your patch has a false positive on > > > > template struct A { }; > > template void f() > > { > >A(); > > }; > > > > which my patch checks in_template_argument_list_p to avoid, though > > checking any_object_scope also currently avoids it. > > > > What do you think? > > I decided that it made more sense to keep the check in > cp_parser_id_expression like you had it, but I moved it to the end to > simplify the logic. Here's what I'm applying, thanks!
Re: [PATCH] c++: error message for dependent template members [PR70417]
On Sat, Jan 15, 2022 at 3:28 AM Anthony Sharp wrote: > Hi Jason, > > Hope you are well. Apologies, I've not had time to sit down and look at > this since last month I quit my old job, then I had family around for the > whole of the Christmas period, and then even more recently I've had to > start my new job. > > In any case happy that you managed to figure it all out. I noticed the > small regression at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104025. > To be honest I wouldn't even know how to begin to go about fixing that so > perhaps it's best if I leave that to you? I guess it's not playing well > with the use of warn_missing_template_keyword. Maybe I didn't set that > variable up properly or something. > FYI it seems to be a latent bug that cp_rollback_tokens doesn't also roll back input_location, not a bug in the new code. > Kind regards, > Anthony > > On Thu, 13 Jan 2022 at 21:01, Jason Merrill wrote: > >> On 12/9/21 10:51, Jason Merrill wrote: >> > On 12/4/21 12:23, Anthony Sharp wrote: >> >> Hi Jason, >> >> >> >> Hope you are well. Apologies for not coming back sooner. >> >> >> >> >I'd put it just above the definition of saved_token_sentinel in >> >> parser.c. >> >> >> >> Sounds good, done. >> >> >> >> >Maybe cp_parser_require_end_of_template_parameter_list? Either way >> >> is fine. >> >> >> >> Even better, have changed it. >> >> >> >> >Hmm, good point; operators that are member functions must be >> >> non-static, >> >> >so we couldn't be doing a comparison of the address of the function. >> >> >> >> In that case I have set it to return early there. >> >> >> >> >So declarator_p should be true there. I'll fix that. >> >> >> >> Thank you. >> >> >> >> >> + if (next_token->keyword == RID_TEMPLATE) >> >> >> +{ >> >> >> + /* But at least make sure it's properly formed (e.g. see >> >> PR19397). */ >> >> >> + if (cp_lexer_peek_nth_token (parser->lexer, 2)->type == >> >> CPP_NAME) >> >> >> + return 1; >> >> >> + >> >> >> + return -1; >> >> >> +} >> >> >> + >> >> >> + /* Could be a ~ referencing the destructor of a class template. >> >> */ >> >> >> + if (next_token->type == CPP_COMPL) >> >> >> +{ >> >> >> + /* It could only be a template. */ >> >> >> + if (cp_lexer_peek_nth_token (parser->lexer, 2)->type == >> >> CPP_NAME) >> >> >> + return 1; >> >> >> + >> >> >> + return -1; >> >> >> +} >> >> > >> >> >Why don't these check for the < ? >> >> >> >> I think perhaps I could have named the function better; instead of >> >> next_token_begins_template_id, how's about >> >> next_token_begins_template_name? >> >> That's all I intended to check for. >> > >> > You're using it to control whether we try to parse a template-id, and >> > it's used to initialize variables named looks_like_template_id, so I >> > think better to keep the old name. >> > >> >> In the first case, something like "->template some_name" will always be >> >> intended as a template, so no need to check for the <. If there were >> >> default >> >> template arguments you could also validly omit the <> completely, I >> think >> >> (could be wrong). >> > >> > Or if the template arguments can be deduced, yes: >> > >> > template struct A >> > { >> >template void f(U u); >> > }; >> > >> > template void g(A a) >> > { >> >a->template f(42); >> > } >> > >> > But 'f' is still not a template-id. >> > >> > ... >> > >> > Actually, it occurs to me that you might be better off handling this in >> > cp_parser_template_name, something like the below, to avoid the complex >> > duplicate logic in the id-expression handling. >> > >> > Note that in this patch I'm using "any_object_scope" as a proxy for >> "I'm >> > parsing an expression", since !is_declaration doesn't work for that; as >> > a result, this doesn't handle the static member function template case. >> > For that you'd probably still need to pass down a flag so that >> > cp_parser_template_name knows it's being called from >> > cp_parser_id_expression. >> > >> > Your patch has a false positive on >> > >> > template struct A { }; >> > template void f() >> > { >> >A(); >> > }; >> > >> > which my patch checks in_template_argument_list_p to avoid, though >> > checking any_object_scope also currently avoids it. >> > >> > What do you think? >> >> I decided that it made more sense to keep the check in >> cp_parser_id_expression like you had it, but I moved it to the end to >> simplify the logic. Here's what I'm applying, thanks! > >