* [PATCH] c++: error message for dependent template members [PR70417]
@ 2021-08-20 16:56 Anthony Sharp
2021-08-27 21:33 ` Jason Merrill
0 siblings, 1 reply; 12+ messages in thread
From: Anthony Sharp @ 2021-08-20 16:56 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 6287 bytes --]
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.
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<class T> void MyDependentTemplate(T t)
{
t.DoSomething<T>(); /* Error - t is dependent. "<" is treated as a
less-than sign because DoSomething is assumed to be a non-template
identifier. */
t.template DoSomething<T>(); // 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>::y = 6;
c.template class_func<Y>();
(&c)->template class_func<Y>();
mytemplateclass<Y>::template class_func_static<Y>();
c2.class_func<myclass>();
(&c2)->class_func<myclass>();
myclass::class_func_static<myclass>();
ai = 6;
bi = 5;
ci = 4;
c.x = 3;
(&c)->x = 2;
mytemplateclass<Y>::y = 1;
c.template class_func<Y>();
(&c)->template class_func<Y>();
mytemplateclass<Y>::template class_func_static<Y>();
c2.class_func<myclass>();
(&c2)->class_func<myclass>();
myclass::class_func_static<myclass>();
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 <class T> static T create();
};
template<class T, class U, class =
decltype(impl::create<T>()->impl::create<U>())>
struct tester{};
GCC currently accepts this code. My patch causes it to be rejected.
For what it's worth, Clang also rejects this code:
1824786093/source.cpp:6:70: error: use 'template' keyword to treat
'create' as a dependent template name
template<class T, class U, class =
decltype(impl::create<T>()->impl::create<U>())>
^ template
I think Clang is correct since from what I understand it should be
written as impl::create<T>()->impl::template create<U>(). Here,
create<T>() is dependent, so it makes sense that we would need
"template" before the scope operator. Then again, I could well be
wrong. The rules around dependent template names are pretty crazy.
REGRESSION TESTING
No differences between clean/patched g++.sum, gcc.sum and
libstdc++.sum. Bootstraps fine. Built on x86_64-pc-linux-gnu for
x86_64-pc-linux-gnu.
CONCLUSION
Some of the changes are a bit debatable, e.g. passing and adding new
arguments to various functions just for the sake of a slight
performance improvement - I don't really have the high-level
experience to be able to judge whether that is worth the increased
code complexity or not, so I just went with my gut.
Please find patch attached :)
Kind regards,
Anthony Sharp
[-- Attachment #2: pr70417.patch --]
[-- Type: text/x-patch, Size: 34195 bytes --]
From 0316e821607f6875293a664ab181747188ef3e73 Mon Sep 17 00:00:00 2001
From: Anthony Sharp <anthonysharp15@gmail.com>
Date: Thu, 19 Aug 2021 17:51:38 +0100
Subject: [PATCH] c++: error message for dependent template members [PR70417]
Add a helpful error 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.
Fix some cases where the compiler was allowing the exclusion
of the template keyword when it shouldn't have been.
gcc/cp/ChangeLog:
2021-08-15 Anthony Sharp <anthonysharp15@gmail.com>
* parser.c (next_token_begins_template_id): New method to
check whether we are looking at what syntactically
looks like a template-id.
(cp_parser_id_expression): Added dependent_expression_p as
function argument. Check whether the id looks like a
template; only attempt to parse it as one if so. Throw
error if missing "template" keyword.
(cp_parser_unqualified_id): Pass looks_like_template through
this function to cp_parser_template_id_expr to avoid repeating
logic unnecessarily.
(cp_parser_postfix_dot_deref_expression): Pass dependent_p
to cp_parser_id_expression.
(cp_parser_template_id): Pass looks_like_template to avoid
repeating logic.
(cp_parser_template_id_expr): As above.
(cp_parser_parse_and_diagnose_invalid_type_name):
Minor update to a function call.
(cp_parser_decltype_expr): As above.
(cp_parser_default_template_template_argument): As above.
(cp_parser_template_argument): As above.
(cp_parser_primary_expression): As above.
(cp_parser_simple_type_specifier): As above.
(cp_parser_pseudo_destructor_name): As above.
(cp_parser_type_name): As above.
(cp_parser_elaborated_type_specifier): As above.
(cp_parser_using_declaration): As above.
(cp_parser_declarator_id): As above.
(cp_parser_class_name): As above.
(cp_parser_class_head): AS above.
(cp_parser_type_requirement): As above.
(cp_parser_constructor_declarator_p): As above.
(cp_parser_omp_var_list_no_open): As above.
(cp_parser_omp_clause_reduction): As above.
(cp_parser_omp_clause_linear): As above.
(cp_parser_omp_clause_detach): As above.
(cp_parser_omp_for_loop_init): As above.
(cp_finish_omp_declare_variant): As above.
(cp_parser_omp_declare_reduction_exprs): As above.
(cp_parser_oacc_routine): As above.
2021-08-15 Anthony Sharp <anthonysharp15@gmail.com>
gcc/ChangeLog:
* symbol-summary.h: Added missing template keyword.
2021-08-15 Anthony Sharp <anthonysharp15@gmail.com>
gcc/testsuite/ChangeLog:
* g++.dg/cpp0x/decltype34.C: Add missing template
keyword(s).
* g++.dg/parse/template26.C: As above.
* g++.old-deja/g++.pt/explicit81.C: As above.
* g++.dg/template/dependent-name15.C: New test for
pr70417.
---
gcc/cp/parser.c | 376 +++++++++++++-----
gcc/symbol-summary.h | 4 +-
gcc/testsuite/g++.dg/cpp0x/decltype34.C | 4 +-
gcc/testsuite/g++.dg/parse/template26.C | 6 +-
.../g++.dg/template/dependent-name15.C | 46 +++
.../g++.old-deja/g++.pt/explicit81.C | 4 +-
6 files changed, 341 insertions(+), 99 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/template/dependent-name15.C
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 45216f0a222..12fa945c1e5 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -2108,9 +2108,9 @@ static void cp_parser_translation_unit (cp_parser *);
static cp_expr cp_parser_primary_expression
(cp_parser *, bool, bool, bool, cp_id_kind *);
static cp_expr cp_parser_id_expression
- (cp_parser *, bool, bool, bool *, bool, bool);
+ (cp_parser *, bool, bool, bool *, bool, bool, bool *);
static cp_expr cp_parser_unqualified_id
- (cp_parser *, bool, bool, bool, bool);
+ (cp_parser *, bool, bool, bool, bool, int);
static tree cp_parser_nested_name_specifier_opt
(cp_parser *, bool, bool, bool, bool, bool = false);
static tree cp_parser_nested_name_specifier
@@ -2438,9 +2438,9 @@ static tree cp_parser_template_parameter
static tree cp_parser_type_parameter
(cp_parser *, bool *);
static tree cp_parser_template_id
- (cp_parser *, bool, bool, enum tag_types, bool);
+ (cp_parser *, bool, bool, enum tag_types, bool, int);
static tree cp_parser_template_id_expr
- (cp_parser *, bool, bool, bool);
+ (cp_parser *, bool, bool, bool, int);
static tree cp_parser_template_name
(cp_parser *, bool, bool, bool, enum tag_types, bool *);
static tree cp_parser_template_argument_list
@@ -3665,7 +3665,8 @@ cp_parser_parse_and_diagnose_invalid_type_name (cp_parser *parser)
/*check_dependency_p=*/true,
/*template_p=*/NULL,
/*declarator_p=*/false,
- /*optional_p=*/false);
+ /*optional_p=*/false,
+ /*dependent_expression_p=*/NULL);
/* If the next token is a (, this is a function with no explicit return
type, i.e. constructor, destructor or conversion op. */
if (cp_lexer_next_token_is (parser->lexer, CPP_OPEN_PAREN)
@@ -5861,7 +5862,8 @@ cp_parser_primary_expression (cp_parser *parser,
/*check_dependency_p=*/true,
&template_p,
/*declarator_p=*/false,
- /*optional_p=*/false);
+ /*optional_p=*/false,
+ /*dependent_expression_p=*/NULL);
if (id_expression == error_mark_node)
return error_mark_node;
id_expr_token = token;
@@ -6028,6 +6030,106 @@ cp_parser_primary_expression (cp_parser *parser,
/*decltype*/false, idk);
}
+/* Check if we are looking at what "looks" like a template-id. Of course,
+ we can't technically know for sure whether it is a template-id without
+ doing lookup (although, the reason we are here is because the context
+ might be dependent and so it might not be possible to do any lookup
+ yet). Return 1 if it does look like a template-id. Return 2
+ if not. */
+static int
+next_token_begins_template_id (cp_parser* parser)
+{
+ cp_token* next_token = cp_lexer_peek_token (parser->lexer);
+ cp_token* following_token;
+ int depth = 1;
+ int i = 3;
+ bool saved = false;
+
+ /* For performance's sake, quickly return from the most common case. */
+ if (next_token->type == CPP_NAME
+ && cp_lexer_peek_nth_token (parser->lexer, 2)->type != CPP_LESS)
+ return 2;
+
+ /* For these purposes we must believe all "template" keywords; identifiers
+ without <> at the end can still be template-ids, but they require the
+ template keyword. The template keyword is the only way we can tell those
+ kinds of ids are template-ids. */
+ 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 2;
+ }
+
+ /* E.g. "::operator- <>". */
+ if (next_token->keyword == RID_OPERATOR
+ && cp_lexer_peek_nth_token (parser->lexer, 3)->type == CPP_LESS)
+ return 1;
+
+ /* Could be a ~ referencing the destructor of a class template.
+ Temporarily eat it up so it's not in our way. */
+ if (next_token->type == CPP_COMPL)
+ {
+ cp_lexer_save_tokens (parser->lexer);
+ cp_lexer_consume_token (parser->lexer);
+ next_token = cp_lexer_peek_token (parser->lexer);
+ saved = true;
+ }
+
+
+ if (next_token->type == CPP_TEMPLATE_ID)
+ goto yes;
+
+ if (next_token->type != CPP_NAME
+ || cp_lexer_peek_nth_token (parser->lexer, 2)->type != CPP_LESS)
+ goto no;
+
+ /* Find offset of final ">". */
+ for (; depth > 0; i++)
+ {
+ switch (cp_lexer_peek_nth_token (parser->lexer, i)->type)
+ {
+ case CPP_LESS:
+ depth++;
+ break;
+ case CPP_GREATER:
+ depth--;
+ break;
+ case CPP_RSHIFT:
+ depth-=2;
+ break;
+ case CPP_EOF:
+ case CPP_SEMICOLON:
+ goto no;
+ default:
+ break;
+ }
+ }
+
+ following_token = cp_lexer_peek_nth_token (parser->lexer, i);
+
+ /* If this is a function template then we would see a "(" after the
+ final ">". It could also be a class template constructor. */
+ if (following_token->type == CPP_OPEN_PAREN
+ /* If this is a class template then we would expect to see ";" or a
+ scope token after the final ">". */
+ || following_token->type == CPP_SEMICOLON
+ || following_token->type == CPP_SCOPE)
+ goto yes;
+
+no:
+ if (saved)
+ cp_lexer_rollback_tokens (parser->lexer);
+ return 2;
+
+yes:
+ if (saved)
+ cp_lexer_rollback_tokens (parser->lexer);
+ return 1;
+}
+
/* Parse an id-expression.
id-expression:
@@ -6060,7 +6162,10 @@ cp_parser_primary_expression (cp_parser *parser,
named is a template.
If DECLARATOR_P is true, the id-expression is appearing as part of
- a declarator, rather than as part of an expression. */
+ a declarator, rather than as part of an expression.
+
+ If DEPENDENT_EXPRESSION_P is true, this is a dependent id-expression
+ that is not the current instantiation. */
static cp_expr
cp_parser_id_expression (cp_parser *parser,
@@ -6068,7 +6173,8 @@ cp_parser_id_expression (cp_parser *parser,
bool check_dependency_p,
bool *template_p,
bool declarator_p,
- bool optional_p)
+ bool optional_p,
+ bool *dependent_expression_p)
{
bool global_scope_p;
bool nested_name_specifier_p;
@@ -6094,6 +6200,51 @@ cp_parser_id_expression (cp_parser *parser,
template_keyword_p)
!= NULL_TREE);
+ /* If this doesn't look like a template-id, there is no point
+ trying to parse it as one. By checking first, we usually speed
+ compilation up, and it allows us to spot missing "template"
+ keywords. */
+ int looks_like_template
+ = next_token_begins_template_id (parser);
+ cp_token* next_token = cp_lexer_peek_token (parser->lexer);
+
+ /* If this is a dependent member-access expression accessing a template
+ member missing the "template" keyword, issue a helpful error message. */
+ if (looks_like_template == 1
+ && !template_keyword_p
+ /* . and -> access tokens. */
+ && ((dependent_expression_p != NULL
+ && *dependent_expression_p)
+ /* :: access expressions. */
+ || (parser->scope
+ && TYPE_P (parser->scope)
+ && dependent_scope_p (parser->scope)))
+ /* ~ before a class template-id disallows the "template" keyword.
+ Probably because in the case of A::~A<>, it is certain that
+ ~A is a template. Note that a destructor cannot actually be a
+ template, but you can call the destructor of a class template - e.g.
+ see "__node->~_Rb_tree_node<_Val>();" in stl_tree.h. */
+ && next_token->type != CPP_COMPL
+ /* A template cannot name a constructor. But don't complain if missing
+ the template keyword in that case; continue on and an error for the
+ (ill-formed) named constructor will get thrown later. */
+ && (!parser->scope
+ || (parser->scope
+ && 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))))
+ {
+ error_at (next_token->location,
+ "expected \"template\" keyword before dependent template "
+ "name");
+
+ /* Skip to the end of the statement to avoid misleading errors. */
+ cp_parser_skip_to_end_of_statement (parser);
+ return error_mark_node;
+ }
+
/* If there is a nested-name-specifier, then we are looking at
the first qualified-id production. */
if (nested_name_specifier_p)
@@ -6117,7 +6268,8 @@ cp_parser_id_expression (cp_parser *parser,
unqualified_id = cp_parser_unqualified_id (parser, *template_p,
check_dependency_p,
declarator_p,
- /*optional_p=*/false);
+ /*optional_p=*/false,
+ looks_like_template);
/* Restore the SAVED_SCOPE for our caller. */
parser->scope = saved_scope;
parser->object_scope = saved_object_scope;
@@ -6129,33 +6281,23 @@ cp_parser_id_expression (cp_parser *parser,
of the other qualified-id productions. */
else if (global_scope_p)
{
- cp_token *token;
- tree id;
-
- /* Peek at the next token. */
- token = cp_lexer_peek_token (parser->lexer);
-
- /* If it's an identifier, and the next token is not a "<", then
- we can avoid the template-id case. This is an optimization
- for this common case. */
- if (token->type == CPP_NAME
- && !cp_parser_nth_token_starts_template_argument_list_p
- (parser, 2))
- return cp_parser_identifier (parser);
-
- cp_parser_parse_tentatively (parser);
- /* Try a template-id. */
- id = cp_parser_template_id_expr (parser,
- /*template_keyword_p=*/false,
- /*check_dependency_p=*/true,
- declarator_p);
- /* If that worked, we're done. */
- if (cp_parser_parse_definitely (parser))
- return id;
+ if (looks_like_template == 1)
+ {
+ cp_parser_parse_tentatively (parser);
+ /* Try a template-id. */
+ tree id = cp_parser_template_id_expr (parser,
+ /*template_keyword_p=*/false,
+ /*check_dependency_p=*/true,
+ declarator_p,
+ looks_like_template);
+ /* If that worked, we're done. */
+ if (cp_parser_parse_definitely (parser))
+ return id;
+ }
/* Peek at the next token. (Changes in the token buffer may
have invalidated the pointer obtained above.) */
- token = cp_lexer_peek_token (parser->lexer);
+ cp_token *token = cp_lexer_peek_token (parser->lexer);
switch (token->type)
{
@@ -6176,7 +6318,8 @@ cp_parser_id_expression (cp_parser *parser,
return cp_parser_unqualified_id (parser, template_keyword_p,
/*check_dependency_p=*/true,
declarator_p,
- optional_p);
+ optional_p,
+ looks_like_template);
}
/* Parse an unqualified-id.
@@ -6191,6 +6334,10 @@ cp_parser_id_expression (cp_parser *parser,
If TEMPLATE_KEYWORD_P is TRUE, we have just seen the `template'
keyword, in a construct like `A::template ...'.
+ If LOOKS_LIKE_TEMPLATE is 1, we checked and saw that this looks
+ like a template-id. If it is 2, we checked and saw that it did
+ not look like a template-id. If 0, we have not checked.
+
Returns a representation of unqualified-id. For the `identifier'
production, an IDENTIFIER_NODE is returned. For the `~ class-name'
production a BIT_NOT_EXPR is returned; the operand of the
@@ -6206,7 +6353,8 @@ cp_parser_unqualified_id (cp_parser* parser,
bool template_keyword_p,
bool check_dependency_p,
bool declarator_p,
- bool optional_p)
+ bool optional_p,
+ int looks_like_template)
{
cp_token *token;
@@ -6219,16 +6367,21 @@ cp_parser_unqualified_id (cp_parser* parser,
{
tree id;
- /* We don't know yet whether or not this will be a
- template-id. */
- cp_parser_parse_tentatively (parser);
- /* Try a template-id. */
- id = cp_parser_template_id_expr (parser, template_keyword_p,
- check_dependency_p,
- declarator_p);
- /* If it worked, we're done. */
- if (cp_parser_parse_definitely (parser))
- return id;
+ if (looks_like_template != 2)
+ {
+ /* We don't know yet whether or not this will be a
+ template-id. */
+ cp_parser_parse_tentatively (parser);
+ /* Try a template-id. */
+ id = cp_parser_template_id_expr (parser,
+ template_keyword_p,
+ check_dependency_p,
+ declarator_p,
+ looks_like_template);
+ /* If it worked, we're done. */
+ if (cp_parser_parse_definitely (parser))
+ return id;
+ }
/* Otherwise, it's an ordinary identifier. */
return cp_parser_identifier (parser);
}
@@ -6236,7 +6389,8 @@ cp_parser_unqualified_id (cp_parser* parser,
case CPP_TEMPLATE_ID:
return cp_parser_template_id_expr (parser, template_keyword_p,
check_dependency_p,
- declarator_p);
+ declarator_p,
+ looks_like_template);
case CPP_COMPL:
{
@@ -6497,7 +6651,8 @@ cp_parser_unqualified_id (cp_parser* parser,
/* Try a template-id. */
id = cp_parser_template_id_expr (parser, template_keyword_p,
/*check_dependency_p=*/true,
- declarator_p);
+ declarator_p,
+ /*looks_like_template=*/0);
/* If that worked, we're done. */
if (cp_parser_parse_definitely (parser))
return id;
@@ -8105,7 +8260,8 @@ cp_parser_postfix_dot_deref_expression (cp_parser *parser,
/*check_dependency_p=*/true,
&template_p,
/*declarator_p=*/false,
- /*optional_p=*/false));
+ /*optional_p=*/false,
+ &dependent_p));
/* In general, build a SCOPE_REF if the member name is qualified.
However, if the name was not dependent and has already been
resolved; there is no need to build the SCOPE_REF. For example;
@@ -8421,7 +8577,8 @@ cp_parser_pseudo_destructor_name (cp_parser* parser,
/*template_keyword_p=*/true,
/*check_dependency_p=*/false,
class_type,
- /*is_declaration=*/true);
+ /*is_declaration=*/true,
+ /*looks_like_template=*/0);
/* Look for the `::' token. */
cp_parser_require (parser, CPP_SCOPE, RT_SCOPE);
}
@@ -15827,7 +15984,8 @@ cp_parser_decltype_expr (cp_parser *parser,
/*check_dependency_p=*/true,
/*template_p=*/NULL,
/*declarator_p=*/false,
- /*optional_p=*/false);
+ /*optional_p=*/false,
+ /*dependent_expression_p=*/NULL);
if (!cp_parser_error_occurred (parser) && expr != error_mark_node)
{
@@ -17321,7 +17479,8 @@ cp_parser_default_template_template_argument (cp_parser *parser)
/*check_dependency_p=*/true,
/*template_p=*/&is_template,
/*declarator_p=*/false,
- /*optional_p=*/false);
+ /*optional_p=*/false,
+ /*dependent_expression_p=*/NULL);
if (TREE_CODE (default_argument) == TYPE_DECL)
/* If the id-expression was a template-id that refers to
a template-class, we already have the declaration here,
@@ -17651,6 +17810,11 @@ cp_parser_type_parameter (cp_parser* parser, bool *is_parameter_pack)
of functions, returns a TEMPLATE_ID_EXPR. If the template-name
names a class, returns a TYPE_DECL for the specialization.
+ If LOOKS_LIKE_TEMPLATE is 1, then we have already discovered
+ that this looks like a template-id. If 2, we have checked and seen
+ that this does not look like a template-id. If it is 0, then we
+ did not check.
+
If CHECK_DEPENDENCY_P is FALSE, names are looked up in
uninstantiated templates. */
@@ -17659,7 +17823,8 @@ cp_parser_template_id (cp_parser *parser,
bool template_keyword_p,
bool check_dependency_p,
enum tag_types tag_type,
- bool is_declaration)
+ bool is_declaration,
+ int looks_like_template)
{
tree templ;
tree arguments;
@@ -17678,15 +17843,20 @@ cp_parser_template_id (cp_parser *parser,
return saved_checks_value (token->u.tree_check_value);
}
- /* Avoid performing name lookup if there is no possibility of
- finding a template-id. */
- if ((token->type != CPP_NAME && token->keyword != RID_OPERATOR)
- || (token->type == CPP_NAME
- && !cp_parser_nth_token_starts_template_argument_list_p
- (parser, 2)))
+ /* Only if we have not already checked whether this looks like a
+ template. */
+ if (looks_like_template == 0)
{
- cp_parser_error (parser, "expected template-id");
- return error_mark_node;
+ /* Avoid performing name lookup if there is no possibility of
+ finding a template-id. */
+ if ((token->type != CPP_NAME && token->keyword != RID_OPERATOR)
+ || (token->type == CPP_NAME
+ && !cp_parser_nth_token_starts_template_argument_list_p
+ (parser, 2)))
+ {
+ cp_parser_error (parser, "expected template-id");
+ return error_mark_node;
+ }
}
/* Remember where the template-id starts. */
@@ -17910,10 +18080,12 @@ static tree
cp_parser_template_id_expr (cp_parser *parser,
bool template_keyword_p,
bool check_dependency_p,
- bool is_declaration)
+ bool is_declaration,
+ int looks_like_template)
{
tree x = cp_parser_template_id (parser, template_keyword_p, check_dependency_p,
- none_type, is_declaration);
+ none_type, is_declaration,
+ looks_like_template);
if (TREE_CODE (x) == TEMPLATE_ID_EXPR
&& concept_check_p (x))
/* We didn't check the arguments in cp_parser_template_id; do that now. */
@@ -18332,7 +18504,8 @@ cp_parser_template_argument (cp_parser* parser)
/*check_dependency_p=*/true,
&template_p,
/*declarator_p=*/false,
- /*optional_p=*/false);
+ /*optional_p=*/false,
+ /*dependent_expression_p=*/NULL);
/* If the next token isn't a `,' or a `>', then this argument wasn't
really finished. */
if (!cp_parser_next_token_ends_template_argument_p (parser))
@@ -19255,7 +19428,8 @@ cp_parser_simple_type_specifier (cp_parser* parser,
/*template_keyword_p=*/true,
/*check_dependency_p=*/true,
none_type,
- /*is_declaration=*/false);
+ /*is_declaration=*/false,
+ /*looks_like_template=*/0);
/* If the template-id did not name a type, we are out of
luck. */
if (TREE_CODE (type) != TYPE_DECL)
@@ -19285,7 +19459,8 @@ cp_parser_simple_type_specifier (cp_parser* parser,
/*template_keyword_p=*/true,
/*check_dependency_p=*/true,
none_type,
- /*is_declaration=*/false);
+ /*is_declaration=*/false,
+ /*looks_like_template=*/0);
/* This is handled below, so back off. */
if (type && concept_check_p (type))
cp_parser_simulate_error (parser);
@@ -19322,7 +19497,8 @@ cp_parser_simple_type_specifier (cp_parser* parser,
/*template_keyword_p=*/false,
/*check_dependency_p=*/true,
none_type,
- /*is_declaration=*/false);
+ /*is_declaration=*/false,
+ /*looks_like_template=*/0);
if (type && concept_check_p (type))
{
location_t loc = EXPR_LOCATION (type);
@@ -19623,7 +19799,8 @@ cp_parser_type_name (cp_parser* parser, bool typename_keyword_p)
/*template_keyword_p=*/false,
/*check_dependency_p=*/true,
none_type,
- /*is_declaration=*/false);
+ /*is_declaration=*/false,
+ /*looks_like_template=*/0);
/* Note that this must be an instantiation of an alias template
because [temp.names]/6 says:
@@ -19865,7 +20042,8 @@ cp_parser_elaborated_type_specifier (cp_parser* parser,
decl = cp_parser_template_id (parser, template_p,
/*check_dependency_p=*/true,
tag_type,
- is_declaration);
+ is_declaration,
+ /*looks_like_template=*/0);
/* If we didn't find a template-id, look for an ordinary
identifier. */
if (!template_p && !cp_parser_parse_definitely (parser))
@@ -21034,7 +21212,8 @@ cp_parser_using_declaration (cp_parser* parser,
/*template_keyword_p=*/false,
/*check_dependency_p=*/true,
/*declarator_p=*/true,
- /*optional_p=*/false);
+ /*optional_p=*/false,
+ /*looks_like_template=*/0);
if (access_declaration_p)
{
@@ -23533,7 +23712,8 @@ cp_parser_declarator_id (cp_parser* parser, bool optional_p)
/*check_dependency_p=*/false,
/*template_p=*/NULL,
/*declarator_p=*/true,
- optional_p);
+ optional_p,
+ /*dependent_expression_p=*/NULL);
if (id && BASELINK_P (id))
id = BASELINK_FUNCTIONS (id);
return id;
@@ -25090,7 +25270,8 @@ cp_parser_class_name (cp_parser *parser,
decl = cp_parser_template_id (parser, template_keyword_p,
check_dependency_p,
tag_type,
- is_declaration);
+ is_declaration,
+ /*looks_like_template=*/0);
if (decl == error_mark_node)
return error_mark_node;
}
@@ -25746,7 +25927,8 @@ cp_parser_class_head (cp_parser* parser,
/*template_keyword_p=*/false,
/*check_dependency_p=*/true,
class_key,
- /*is_declaration=*/true);
+ /*is_declaration=*/true,
+ /*looks_like_template=*/0);
/* If that didn't work, it could still be an identifier. */
if (!cp_parser_parse_definitely (parser))
{
@@ -29532,7 +29714,8 @@ cp_parser_type_requirement (cp_parser *parser)
/*template_keyword_p=*/true,
/*check_dependency=*/false,
/*tag_type=*/none_type,
- /*is_declaration=*/false);
+ /*is_declaration=*/false,
+ /*looks_like_template=*/0);
type = make_typename_type (parser->scope, type, typename_type,
/*complain=*/tf_error);
}
@@ -30253,7 +30436,8 @@ cp_parser_constructor_declarator_p (cp_parser *parser, cp_parser_flags flags,
/*template_keyword_p=*/false,
/*check_dependency_p=*/false,
/*declarator_p=*/true,
- /*optional_p=*/false);
+ /*optional_p=*/false,
+ /*looks_like_template=*/0);
if (is_overloaded_fn (id))
id = DECL_NAME (get_first_fn (id));
if (!constructor_name_p (id, nested_name_specifier))
@@ -35845,7 +36029,8 @@ cp_parser_omp_var_list_no_open (cp_parser *parser, enum omp_clause_code kind,
/*check_dependency_p=*/true,
/*template_p=*/NULL,
/*declarator_p=*/false,
- /*optional_p=*/false);
+ /*optional_p=*/false,
+ /*dependent_expression_p=*/NULL);
if (name == error_mark_node)
{
if ((kind == OMP_CLAUSE_DEPEND || kind == OMP_CLAUSE_AFFINITY)
@@ -37345,7 +37530,8 @@ cp_parser_omp_clause_reduction (cp_parser *parser, enum omp_clause_code kind,
/*check_dependency_p=*/true,
/*template_p=*/NULL,
/*declarator_p=*/false,
- /*optional_p=*/false);
+ /*optional_p=*/false,
+ /*dependent_expression_p=*/NULL);
parser->colon_corrects_to_scope_p = saved_colon_corrects_to_scope_p;
if (identifier_p (id))
{
@@ -37870,7 +38056,8 @@ cp_parser_omp_clause_linear (cp_parser *parser, tree list,
/*check_dependency_p=*/true,
/*template_p=*/NULL,
/*declarator_p=*/false,
- /*optional_p=*/false);
+ /*optional_p=*/false,
+ /*dependent_expression_p=*/NULL);
if (step != error_mark_node)
step = cp_parser_lookup_name_simple (parser, step, token->location);
if (step == error_mark_node)
@@ -38066,7 +38253,8 @@ cp_parser_omp_clause_detach (cp_parser *parser, tree list)
/*check_dependency_p=*/true,
/*template_p=*/NULL,
/*declarator_p=*/false,
- /*optional_p=*/false);
+ /*optional_p=*/false,
+ /*dependent_expression_p=*/NULL);
if (name == error_mark_node)
decl = error_mark_node;
else
@@ -40509,11 +40697,14 @@ cp_parser_omp_for_loop_init (cp_parser *parser,
cp_parser_abort_tentative_parse (parser);
cp_parser_parse_tentatively (parser);
cp_token *token = cp_lexer_peek_token (parser->lexer);
- tree name = cp_parser_id_expression (parser, /*template_p=*/false,
- /*check_dependency_p=*/true,
- /*template_p=*/NULL,
- /*declarator_p=*/false,
- /*optional_p=*/false);
+ tree name
+ = cp_parser_id_expression (parser,
+ /*template_p=*/false,
+ /*check_dependency_p=*/true,
+ /*template_p=*/NULL,
+ /*declarator_p=*/false,
+ /*optional_p=*/false,
+ /*dependent_expression_p=*/NULL);
if (name != error_mark_node
&& last_tok == cp_lexer_peek_token (parser->lexer))
{
@@ -43831,7 +44022,8 @@ cp_finish_omp_declare_variant (cp_parser *parser, cp_token *pragma_tok,
/*check_dependency_p=*/true,
/*template_p=*/&template_p,
/*declarator_p=*/false,
- /*optional_p=*/false);
+ /*optional_p=*/false,
+ /*dependent_expression_p=*/NULL);
parens.require_close (parser);
tree variant;
@@ -44252,11 +44444,14 @@ cp_parser_omp_declare_reduction_exprs (tree fndecl, cp_parser *parser)
cp_parser_parse_tentatively (parser);
/* Don't create location wrapper nodes here. */
auto_suppress_location_wrappers sentinel;
- tree fn_name = cp_parser_id_expression (parser, /*template_p=*/false,
- /*check_dependency_p=*/true,
- /*template_p=*/NULL,
- /*declarator_p=*/false,
- /*optional_p=*/false);
+ tree fn_name
+ = cp_parser_id_expression (parser,
+ /*template_p=*/false,
+ /*check_dependency_p=*/true,
+ /*template_p=*/NULL,
+ /*declarator_p=*/false,
+ /*optional_p=*/false,
+ /*dependent_expression_p=*/NULL);
vec<tree, va_gc> *args;
if (fn_name == error_mark_node
|| cp_parser_error_occurred (parser)
@@ -44916,7 +45111,8 @@ cp_parser_oacc_routine (cp_parser *parser, cp_token *pragma_tok,
/*check_dependency_p=*/false,
/*template_p=*/NULL,
/*declarator_p=*/false,
- /*optional_p=*/false);
+ /*optional_p=*/false,
+ /*dependent_expression_p=*/NULL);
tree decl = (identifier_p (name)
? cp_parser_lookup_name_simple (parser, name, name_loc)
: name);
diff --git a/gcc/symbol-summary.h b/gcc/symbol-summary.h
index 6c0fbdd1c4a..aa8a7725bc4 100644
--- a/gcc/symbol-summary.h
+++ b/gcc/symbol-summary.h
@@ -191,7 +191,7 @@ public:
template<typename Arg, bool (*f)(const T &, Arg)>
void traverse (Arg a) const
{
- m_map.traverse <f> (a);
+ m_map.template traverse <f> (a);
}
/* Getter for summary callgraph node pointer. If a summary for a node
@@ -690,7 +690,7 @@ public:
template<typename Arg, bool (*f)(const T &, Arg)>
void traverse (Arg a) const
{
- m_map.traverse <f> (a);
+ m_map.template traverse <f> (a);
}
/* Getter for summary callgraph edge pointer.
diff --git a/gcc/testsuite/g++.dg/cpp0x/decltype34.C b/gcc/testsuite/g++.dg/cpp0x/decltype34.C
index 028e50669f9..b5e7a5cba0c 100644
--- a/gcc/testsuite/g++.dg/cpp0x/decltype34.C
+++ b/gcc/testsuite/g++.dg/cpp0x/decltype34.C
@@ -7,13 +7,13 @@ struct impl
};
template<class T, class U,
- class = decltype(impl::create<T>()->impl::create<U>())>
+ class = decltype(impl::create<T>()->impl::template create<U>())>
struct tester{};
tester<impl*, int> ti;
template<class T, class U,
- class = decltype(impl::create<T>()->impl::create<U>())>
+ class = decltype(impl::create<T>()->impl::template create<U>())>
int test() { return 0; }
int i = test<impl*, int>();
diff --git a/gcc/testsuite/g++.dg/parse/template26.C b/gcc/testsuite/g++.dg/parse/template26.C
index aab9763ccaf..add8c64eabb 100644
--- a/gcc/testsuite/g++.dg/parse/template26.C
+++ b/gcc/testsuite/g++.dg/parse/template26.C
@@ -6,13 +6,13 @@ namespace impl
}
template <class T, class U, __SIZE_TYPE__
- = sizeof(impl::create<T>()->*impl::create<U>())>
+ = sizeof(impl::create<T>()->*impl::template create<U>())>
struct foo1;
template <class T, class U, __SIZE_TYPE__
- = sizeof(impl::create<T>()->impl::create<U>())> // { dg-error "not a class member" }
+ = sizeof(impl::create<T>()->impl::template create<U>())> // { dg-error "not a class member" }
struct foo2;
template <class T, class U, __SIZE_TYPE__
- = sizeof(impl::create<T>().impl::create<U>())> // { dg-error "not a class member" }
+ = sizeof(impl::create<T>().impl::template create<U>())> // { dg-error "not a class member" }
struct foo3;
diff --git a/gcc/testsuite/g++.dg/template/dependent-name15.C b/gcc/testsuite/g++.dg/template/dependent-name15.C
new file mode 100644
index 00000000000..e03971f9b2f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/dependent-name15.C
@@ -0,0 +1,46 @@
+// C++ PR 70317
+// { dg-do compile }
+
+template<class T> class mytemplateclass
+{
+ public:
+ template<class U> void class_func() {}
+ template<class U> static void class_func_static() {}
+};
+
+class myclass
+{
+ public:
+ int testint;
+ template<class U> void class_func() {}
+ template<class U> static void class_func_static() {}
+};
+
+template<class Y> void tests_func(mytemplateclass<Y> c, myclass c2)
+{
+ /* Dependent template accessors (ill-formed code). */
+ c.class_func<Y>(); // { dg-error "keyword before dependent template name" }
+ (&c)->class_func<Y>(); // { dg-error "keyword before dependent template name" }
+ mytemplateclass<Y>::class_func_static<Y>(); // { dg-error "keyword before dependent template name" }
+
+ /* Dependent template accessors (well-formed code). */
+ c.template class_func<Y>();
+ (&c)->template class_func<Y>();
+ mytemplateclass<Y>::template class_func_static<Y>();
+
+ /* Non-dependent template accessors (well-formed code). */
+ c2.class_func<myclass>();
+ (&c2)->class_func<myclass>();
+ myclass::class_func_static<myclass>();
+}
+
+int main()
+{
+ mytemplateclass<myclass> c;
+ myclass c2;
+ tests_func<myclass>(c, c2);
+
+ c2.testint = 53;
+ /* Make sure this isn't treated as a template. */
+ bool testbool = c2.testint < 999 > 7;
+}
\ No newline at end of file
diff --git a/gcc/testsuite/g++.old-deja/g++.pt/explicit81.C b/gcc/testsuite/g++.old-deja/g++.pt/explicit81.C
index 576ba5439a6..5232d770720 100644
--- a/gcc/testsuite/g++.old-deja/g++.pt/explicit81.C
+++ b/gcc/testsuite/g++.old-deja/g++.pt/explicit81.C
@@ -26,10 +26,10 @@ void tf(C *ptr)
{
N::nf<N::e0>();
gf<N::e0>();
- ptr->X::xfn <N::e0> ();
+ ptr->X::template xfn <N::e0> ();
ptr->C::template xfn <N::e0> ();
ptr->template xfn <N::e0> ();
- ptr->X::sfn <N::e0> ();
+ ptr->X::template sfn <N::e0> ();
ptr->C::template sfn <N::e0> ();
ptr->template sfn <N::e0> ();
X::sfn <N::e0> ();
--
2.32.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] c++: error message for dependent template members [PR70417]
2021-08-20 16:56 [PATCH] c++: error message for dependent template members [PR70417] Anthony Sharp
@ 2021-08-27 21:33 ` Jason Merrill
[not found] ` <CA+Lh_AnijQzyM6qgwHS7hZqmA8uO8VahG5fmdqFMF6wRcrbefQ@mail.gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Jason Merrill @ 2021-08-27 21:33 UTC (permalink / raw)
To: Anthony Sharp, gcc-patches; +Cc: Marek Polacek
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<class T> void MyDependentTemplate(T t)
> {
> t.DoSomething<T>(); /* Error - t is dependent. "<" is treated as a
> less-than sign because DoSomething is assumed to be a non-template
> identifier. */
> t.template DoSomething<T>(); // 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>::y = 6;
> c.template class_func<Y>();
> (&c)->template class_func<Y>();
> mytemplateclass<Y>::template class_func_static<Y>();
> c2.class_func<myclass>();
> (&c2)->class_func<myclass>();
> myclass::class_func_static<myclass>();
> ai = 6;
> bi = 5;
> ci = 4;
> c.x = 3;
> (&c)->x = 2;
> mytemplateclass<Y>::y = 1;
> c.template class_func<Y>();
> (&c)->template class_func<Y>();
> mytemplateclass<Y>::template class_func_static<Y>();
> c2.class_func<myclass>();
> (&c2)->class_func<myclass>();
> myclass::class_func_static<myclass>();
>
> 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 <class T> static T create();
> };
>
> template<class T, class U, class =
> decltype(impl::create<T>()->impl::create<U>())>
> struct tester{};
>
> GCC currently accepts this code. My patch causes it to be rejected.
> For what it's worth, Clang also rejects this code:
>
> 1824786093/source.cpp:6:70: error: use 'template' keyword to treat
> 'create' as a dependent template name
> template<class T, class U, class =
> decltype(impl::create<T>()->impl::create<U>())>
>
> ^ template
>
> I think Clang is correct since from what I understand it should be
> written as impl::create<T>()->impl::template create<U>(). Here,
> create<T>() is dependent, so it makes sense that we would need
> "template" before the scope operator. Then again, I could well be
> wrong. The rules around dependent template names are pretty crazy.
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
Previously, e.g. in C++20,
https://timsong-cpp.github.io/cppwp/n4861/basic.lookup.classref#4 said
that when we see ->impl, since we can't look up the name in the
(dependent) object type, we do unqualified lookup and find ::impl, so
create<U> is not in a dependent scope, and the testcase is fine.
The 1835 change clarifies that we only do unqualified lookup of the
second impl if the object type is non-dependent, so the second create is
in a dependent scope, and we need the ::template.
But in either case, whether create<U> 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.
1835 also makes the following testcase valid, which we don't currently
accept, but clang does:
template <class T> void f(T t) { t.foo::bar(); }
struct foo { void bar(); };
int main() { f(foo()); }
For C++20 and down, I want to start accepting this testcase, but also
still accept decltype34.C to avoid breaking existing code. But that's a
separate issue; I don't think your patch should affect decltype34.
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 <class T, int N> void f(T t) { t.m<N>(0); }
struct A { int m; } a;
int main() { f<A,42>(a); }
Comparing the result of < by > is very unlikely to be what the user
actually meant, but it is potentially valid. So we should accept f with
a warning about the dubious expression. People can silence the warning
if they really mean this by parenthesizing the LHS of the < operator.
Another valid program, using ::
int x;
template <class T, int N> void f(T t) { t.m<N>::x; }
struct A { int m; } a;
int main() { f<A,42>(a); }
Now to reviewing the actual patch:
> + yet). Return 1 if it does look like a template-id. Return 2
> + if not. */
Let's use -1 for definitely not.
> + /* Could be a ~ referencing the destructor of a class template.
> + Temporarily eat it up so it's not in our way. */
> + if (next_token->type == CPP_COMPL)
> + {
> + cp_lexer_save_tokens (parser->lexer);
> + cp_lexer_consume_token (parser->lexer);
> + next_token = cp_lexer_peek_token (parser->lexer);
> + saved = true;
> + }
There's no ambiguity in the case of a destructor, as you note in your
comments in cp_parser_id_expression. How about returning early if we see ~?
> + /* Find offset of final ">". */
> + for (; depth > 0; i++)
> + {
> + switch (cp_lexer_peek_nth_token (parser->lexer, i)->type)
> + {
> + case CPP_LESS:
> + depth++;
> + break;
> + case CPP_GREATER:
> + depth--;
> + break;
> + case CPP_RSHIFT:
> + depth-=2;
> + break;
> + case CPP_EOF:
> + case CPP_SEMICOLON:
> + goto no;
> + default:
> + break;
> + }
> + }
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.
> +no:
> + if (saved)
> + cp_lexer_rollback_tokens (parser->lexer);
> + return 2;
> +
> +yes:
> + if (saved)
> + cp_lexer_rollback_tokens (parser->lexer);
> + return 1;
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.
> + If DEPENDENT_EXPRESSION_P is true, this is a dependent id-expression
> + that is not the current instantiation. */
As mentioned above, let's not add this parameter.
> + error_at (next_token->location,
> + "expected \"template\" keyword before dependent template "
> + "name");
As mentioned above, this should be a warning.
> - /* If it worked, we're done. */
> - if (cp_parser_parse_definitely (parser))
> - return id;
> + if (looks_like_template != 2)
> + {
> + /* We don't know yet whether or not this will be a
> + template-id. */
The indentation here is off.
> + int looks_like_template)
Let's give these parms a default argument of 0. And call them
looks_like_template_id to be clearer.
> - /* Avoid performing name lookup if there is no possibility of
> - finding a template-id. */
> - if ((token->type != CPP_NAME && token->keyword != RID_OPERATOR)
> - || (token->type == CPP_NAME
> - && !cp_parser_nth_token_starts_template_argument_list_p
> - (parser, 2)))
> + /* Only if we have not already checked whether this looks like a
> + template. */
> + if (looks_like_template == 0)
> {
> - cp_parser_error (parser, "expected template-id");
> - return error_mark_node;
> + /* Avoid performing name lookup if there is no possibility of
> + finding a template-id. */
You could add the looks_like_template_id check to the existing condition
so we don't need to reindent the comment or body.
> +++ b/gcc/symbol-summary.h
> @@ -191,7 +191,7 @@ public:
> template<typename Arg, bool (*f)(const T &, Arg)>
> void traverse (Arg a) const
> {
> - m_map.traverse <f> (a);
> + m_map.template traverse <f> (a);
I've gone ahead and applied this fix as a separate patch.
Jason
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] c++: error message for dependent template members [PR70417]
[not found] ` <CA+Lh_AnijQzyM6qgwHS7hZqmA8uO8VahG5fmdqFMF6wRcrbefQ@mail.gmail.com>
@ 2021-09-17 22:17 ` Anthony Sharp
2021-09-17 22:22 ` Anthony Sharp
0 siblings, 1 reply; 12+ messages in thread
From: Anthony Sharp @ 2021-09-17 22:17 UTC (permalink / raw)
To: Jason Merrill, gcc-patches
Re-adding gcc-patches@gcc.gnu.org.
---------- Forwarded message ---------
From: Anthony Sharp <anthonysharp15@gmail.com>
Date: Fri, 17 Sept 2021 at 23:11
Subject: Re: [PATCH] c++: error message for dependent template members [PR70417]
To: Jason Merrill <jason@redhat.com>
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<U> 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 <class T, int N> void f(T t) { t.m<N>(0); }
> struct A { int m; } a;
> int main() { f<A,42>(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.
Thanks for the help,
Anthony
On Fri, 27 Aug 2021 at 22:33, Jason Merrill <jason@redhat.com> wrote:
>
> 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<class T> void MyDependentTemplate(T t)
> > {
> > t.DoSomething<T>(); /* Error - t is dependent. "<" is treated as a
> > less-than sign because DoSomething is assumed to be a non-template
> > identifier. */
> > t.template DoSomething<T>(); // 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>::y = 6;
> > c.template class_func<Y>();
> > (&c)->template class_func<Y>();
> > mytemplateclass<Y>::template class_func_static<Y>();
> > c2.class_func<myclass>();
> > (&c2)->class_func<myclass>();
> > myclass::class_func_static<myclass>();
> > ai = 6;
> > bi = 5;
> > ci = 4;
> > c.x = 3;
> > (&c)->x = 2;
> > mytemplateclass<Y>::y = 1;
> > c.template class_func<Y>();
> > (&c)->template class_func<Y>();
> > mytemplateclass<Y>::template class_func_static<Y>();
> > c2.class_func<myclass>();
> > (&c2)->class_func<myclass>();
> > myclass::class_func_static<myclass>();
> >
> > 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 <class T> static T create();
> > };
> >
> > template<class T, class U, class =
> > decltype(impl::create<T>()->impl::create<U>())>
> > struct tester{};
> >
> > GCC currently accepts this code. My patch causes it to be rejected.
>
> > For what it's worth, Clang also rejects this code:
> >
> > 1824786093/source.cpp:6:70: error: use 'template' keyword to treat
> > 'create' as a dependent template name
> > template<class T, class U, class =
> > decltype(impl::create<T>()->impl::create<U>())>
> >
> > ^ template
> >
> > I think Clang is correct since from what I understand it should be
> > written as impl::create<T>()->impl::template create<U>(). Here,
> > create<T>() is dependent, so it makes sense that we would need
> > "template" before the scope operator. Then again, I could well be
> > wrong. The rules around dependent template names are pretty crazy.
>
> 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
>
> Previously, e.g. in C++20,
> https://timsong-cpp.github.io/cppwp/n4861/basic.lookup.classref#4 said
> that when we see ->impl, since we can't look up the name in the
> (dependent) object type, we do unqualified lookup and find ::impl, so
> create<U> is not in a dependent scope, and the testcase is fine.
>
> The 1835 change clarifies that we only do unqualified lookup of the
> second impl if the object type is non-dependent, so the second create is
> in a dependent scope, and we need the ::template.
>
> But in either case, whether create<U> 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.
>
> 1835 also makes the following testcase valid, which we don't currently
> accept, but clang does:
>
> template <class T> void f(T t) { t.foo::bar(); }
> struct foo { void bar(); };
> int main() { f(foo()); }
>
> For C++20 and down, I want to start accepting this testcase, but also
> still accept decltype34.C to avoid breaking existing code. But that's a
> separate issue; I don't think your patch should affect decltype34.
>
> 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 <class T, int N> void f(T t) { t.m<N>(0); }
> struct A { int m; } a;
> int main() { f<A,42>(a); }
>
> Comparing the result of < by > is very unlikely to be what the user
> actually meant, but it is potentially valid. So we should accept f with
> a warning about the dubious expression. People can silence the warning
> if they really mean this by parenthesizing the LHS of the < operator.
>
> Another valid program, using ::
>
> int x;
> template <class T, int N> void f(T t) { t.m<N>::x; }
> struct A { int m; } a;
> int main() { f<A,42>(a); }
>
> Now to reviewing the actual patch:
>
> > + yet). Return 1 if it does look like a template-id. Return 2
> > + if not. */
>
> Let's use -1 for definitely not.
>
> > + /* Could be a ~ referencing the destructor of a class template.
> > + Temporarily eat it up so it's not in our way. */
> > + if (next_token->type == CPP_COMPL)
> > + {
> > + cp_lexer_save_tokens (parser->lexer);
> > + cp_lexer_consume_token (parser->lexer);
> > + next_token = cp_lexer_peek_token (parser->lexer);
> > + saved = true;
> > + }
>
> There's no ambiguity in the case of a destructor, as you note in your
> comments in cp_parser_id_expression. How about returning early if we see ~?
>
> > + /* Find offset of final ">". */
> > + for (; depth > 0; i++)
> > + {
> > + switch (cp_lexer_peek_nth_token (parser->lexer, i)->type)
> > + {
> > + case CPP_LESS:
> > + depth++;
> > + break;
> > + case CPP_GREATER:
> > + depth--;
> > + break;
> > + case CPP_RSHIFT:
> > + depth-=2;
> > + break;
> > + case CPP_EOF:
> > + case CPP_SEMICOLON:
> > + goto no;
> > + default:
> > + break;
> > + }
> > + }
>
> 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.
>
> > +no:
> > + if (saved)
> > + cp_lexer_rollback_tokens (parser->lexer);
> > + return 2;
> > +
> > +yes:
> > + if (saved)
> > + cp_lexer_rollback_tokens (parser->lexer);
> > + return 1;
>
> 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.
>
> > + If DEPENDENT_EXPRESSION_P is true, this is a dependent id-expression
> > + that is not the current instantiation. */
>
> As mentioned above, let's not add this parameter.
>
> > + error_at (next_token->location,
> > + "expected \"template\" keyword before dependent template "
> > + "name");
>
> As mentioned above, this should be a warning.
>
> > - /* If it worked, we're done. */
> > - if (cp_parser_parse_definitely (parser))
> > - return id;
> > + if (looks_like_template != 2)
> > + {
> > + /* We don't know yet whether or not this will be a
> > + template-id. */
>
> The indentation here is off.
>
> > + int looks_like_template)
>
> Let's give these parms a default argument of 0. And call them
> looks_like_template_id to be clearer.
>
> > - /* Avoid performing name lookup if there is no possibility of
> > - finding a template-id. */
> > - if ((token->type != CPP_NAME && token->keyword != RID_OPERATOR)
> > - || (token->type == CPP_NAME
> > - && !cp_parser_nth_token_starts_template_argument_list_p
> > - (parser, 2)))
> > + /* Only if we have not already checked whether this looks like a
> > + template. */
> > + if (looks_like_template == 0)
> > {
> > - cp_parser_error (parser, "expected template-id");
> > - return error_mark_node;
> > + /* Avoid performing name lookup if there is no possibility of
> > + finding a template-id. */
>
> You could add the looks_like_template_id check to the existing condition
> so we don't need to reindent the comment or body.
>
> > +++ b/gcc/symbol-summary.h
> > @@ -191,7 +191,7 @@ public:
> > template<typename Arg, bool (*f)(const T &, Arg)>
> > void traverse (Arg a) const
> > {
> > - m_map.traverse <f> (a);
> > + m_map.template traverse <f> (a);
>
> I've gone ahead and applied this fix as a separate patch.
>
> Jason
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] c++: error message for dependent template members [PR70417]
2021-09-17 22:17 ` Anthony Sharp
@ 2021-09-17 22:22 ` Anthony Sharp
2021-09-22 20:04 ` Jason Merrill
0 siblings, 1 reply; 12+ messages in thread
From: Anthony Sharp @ 2021-09-17 22:22 UTC (permalink / raw)
To: Jason Merrill, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 18270 bytes --]
And also re-attaching the patch!
On Fri, 17 Sept 2021 at 23:17, Anthony Sharp <anthonysharp15@gmail.com> wrote:
>
> Re-adding gcc-patches@gcc.gnu.org.
>
> ---------- Forwarded message ---------
> From: Anthony Sharp <anthonysharp15@gmail.com>
> Date: Fri, 17 Sept 2021 at 23:11
> Subject: Re: [PATCH] c++: error message for dependent template members [PR70417]
> To: Jason Merrill <jason@redhat.com>
>
>
> 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<U> 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 <class T, int N> void f(T t) { t.m<N>(0); }
> > struct A { int m; } a;
> > int main() { f<A,42>(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.
>
> Thanks for the help,
> Anthony
>
>
>
> On Fri, 27 Aug 2021 at 22:33, Jason Merrill <jason@redhat.com> wrote:
> >
> > 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<class T> void MyDependentTemplate(T t)
> > > {
> > > t.DoSomething<T>(); /* Error - t is dependent. "<" is treated as a
> > > less-than sign because DoSomething is assumed to be a non-template
> > > identifier. */
> > > t.template DoSomething<T>(); // 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>::y = 6;
> > > c.template class_func<Y>();
> > > (&c)->template class_func<Y>();
> > > mytemplateclass<Y>::template class_func_static<Y>();
> > > c2.class_func<myclass>();
> > > (&c2)->class_func<myclass>();
> > > myclass::class_func_static<myclass>();
> > > ai = 6;
> > > bi = 5;
> > > ci = 4;
> > > c.x = 3;
> > > (&c)->x = 2;
> > > mytemplateclass<Y>::y = 1;
> > > c.template class_func<Y>();
> > > (&c)->template class_func<Y>();
> > > mytemplateclass<Y>::template class_func_static<Y>();
> > > c2.class_func<myclass>();
> > > (&c2)->class_func<myclass>();
> > > myclass::class_func_static<myclass>();
> > >
> > > 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 <class T> static T create();
> > > };
> > >
> > > template<class T, class U, class =
> > > decltype(impl::create<T>()->impl::create<U>())>
> > > struct tester{};
> > >
> > > GCC currently accepts this code. My patch causes it to be rejected.
> >
> > > For what it's worth, Clang also rejects this code:
> > >
> > > 1824786093/source.cpp:6:70: error: use 'template' keyword to treat
> > > 'create' as a dependent template name
> > > template<class T, class U, class =
> > > decltype(impl::create<T>()->impl::create<U>())>
> > >
> > > ^ template
> > >
> > > I think Clang is correct since from what I understand it should be
> > > written as impl::create<T>()->impl::template create<U>(). Here,
> > > create<T>() is dependent, so it makes sense that we would need
> > > "template" before the scope operator. Then again, I could well be
> > > wrong. The rules around dependent template names are pretty crazy.
> >
> > 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
> >
> > Previously, e.g. in C++20,
> > https://timsong-cpp.github.io/cppwp/n4861/basic.lookup.classref#4 said
> > that when we see ->impl, since we can't look up the name in the
> > (dependent) object type, we do unqualified lookup and find ::impl, so
> > create<U> is not in a dependent scope, and the testcase is fine.
> >
> > The 1835 change clarifies that we only do unqualified lookup of the
> > second impl if the object type is non-dependent, so the second create is
> > in a dependent scope, and we need the ::template.
> >
> > But in either case, whether create<U> 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.
> >
> > 1835 also makes the following testcase valid, which we don't currently
> > accept, but clang does:
> >
> > template <class T> void f(T t) { t.foo::bar(); }
> > struct foo { void bar(); };
> > int main() { f(foo()); }
> >
> > For C++20 and down, I want to start accepting this testcase, but also
> > still accept decltype34.C to avoid breaking existing code. But that's a
> > separate issue; I don't think your patch should affect decltype34.
> >
> > 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 <class T, int N> void f(T t) { t.m<N>(0); }
> > struct A { int m; } a;
> > int main() { f<A,42>(a); }
> >
> > Comparing the result of < by > is very unlikely to be what the user
> > actually meant, but it is potentially valid. So we should accept f with
> > a warning about the dubious expression. People can silence the warning
> > if they really mean this by parenthesizing the LHS of the < operator.
> >
> > Another valid program, using ::
> >
> > int x;
> > template <class T, int N> void f(T t) { t.m<N>::x; }
> > struct A { int m; } a;
> > int main() { f<A,42>(a); }
> >
> > Now to reviewing the actual patch:
> >
> > > + yet). Return 1 if it does look like a template-id. Return 2
> > > + if not. */
> >
> > Let's use -1 for definitely not.
> >
> > > + /* Could be a ~ referencing the destructor of a class template.
> > > + Temporarily eat it up so it's not in our way. */
> > > + if (next_token->type == CPP_COMPL)
> > > + {
> > > + cp_lexer_save_tokens (parser->lexer);
> > > + cp_lexer_consume_token (parser->lexer);
> > > + next_token = cp_lexer_peek_token (parser->lexer);
> > > + saved = true;
> > > + }
> >
> > There's no ambiguity in the case of a destructor, as you note in your
> > comments in cp_parser_id_expression. How about returning early if we see ~?
> >
> > > + /* Find offset of final ">". */
> > > + for (; depth > 0; i++)
> > > + {
> > > + switch (cp_lexer_peek_nth_token (parser->lexer, i)->type)
> > > + {
> > > + case CPP_LESS:
> > > + depth++;
> > > + break;
> > > + case CPP_GREATER:
> > > + depth--;
> > > + break;
> > > + case CPP_RSHIFT:
> > > + depth-=2;
> > > + break;
> > > + case CPP_EOF:
> > > + case CPP_SEMICOLON:
> > > + goto no;
> > > + default:
> > > + break;
> > > + }
> > > + }
> >
> > 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.
> >
> > > +no:
> > > + if (saved)
> > > + cp_lexer_rollback_tokens (parser->lexer);
> > > + return 2;
> > > +
> > > +yes:
> > > + if (saved)
> > > + cp_lexer_rollback_tokens (parser->lexer);
> > > + return 1;
> >
> > 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.
> >
> > > + If DEPENDENT_EXPRESSION_P is true, this is a dependent id-expression
> > > + that is not the current instantiation. */
> >
> > As mentioned above, let's not add this parameter.
> >
> > > + error_at (next_token->location,
> > > + "expected \"template\" keyword before dependent template "
> > > + "name");
> >
> > As mentioned above, this should be a warning.
> >
> > > - /* If it worked, we're done. */
> > > - if (cp_parser_parse_definitely (parser))
> > > - return id;
> > > + if (looks_like_template != 2)
> > > + {
> > > + /* We don't know yet whether or not this will be a
> > > + template-id. */
> >
> > The indentation here is off.
> >
> > > + int looks_like_template)
> >
> > Let's give these parms a default argument of 0. And call them
> > looks_like_template_id to be clearer.
> >
> > > - /* Avoid performing name lookup if there is no possibility of
> > > - finding a template-id. */
> > > - if ((token->type != CPP_NAME && token->keyword != RID_OPERATOR)
> > > - || (token->type == CPP_NAME
> > > - && !cp_parser_nth_token_starts_template_argument_list_p
> > > - (parser, 2)))
> > > + /* Only if we have not already checked whether this looks like a
> > > + template. */
> > > + if (looks_like_template == 0)
> > > {
> > > - cp_parser_error (parser, "expected template-id");
> > > - return error_mark_node;
> > > + /* Avoid performing name lookup if there is no possibility of
> > > + finding a template-id. */
> >
> > You could add the looks_like_template_id check to the existing condition
> > so we don't need to reindent the comment or body.
> >
> > > +++ b/gcc/symbol-summary.h
> > > @@ -191,7 +191,7 @@ public:
> > > template<typename Arg, bool (*f)(const T &, Arg)>
> > > void traverse (Arg a) const
> > > {
> > > - m_map.traverse <f> (a);
> > > + m_map.template traverse <f> (a);
> >
> > I've gone ahead and applied this fix as a separate patch.
> >
> > Jason
> >
[-- Attachment #2: pr70417_5.patch --]
[-- Type: application/octet-stream, Size: 33558 bytes --]
From bd0d6f252dbe8423fc9bccad76f1173f1f451f47 Mon Sep 17 00:00:00 2001
From: Anthony Sharp <anthonysharp15@gmail.com>
Date: Fri, 17 Sep 2021 22:26:30 +0100
Subject: [PATCH] c++: warning for dependent template members [PR70417]
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.
Correct some cases where the template keyword was missing when
it shouldn't have been.
gcc/c-family/ChangeLog:
2021-09-17 Anthony Sharp <anthonysharp15@gmail.com>
* c.opt: Added -Wmissing-template-keyword.
gcc/cp/ChangeLog:
2021-09-17 Anthony Sharp <anthonysharp15@gmail.com>
* parser.c (struct saved_token_sentinel): Rolls-back
by default. Added modes to control what happens on
destruction.
(next_token_begins_template_id): New method to
check whether we are looking at what syntactically
looks like a template-id.
(cp_parser_id_expression): Check whether the id looks like a
template; only attempt to parse it as one if so. Give
warning if missing "template" keyword. Refactor template logic.
(cp_parser_unqualified_id): Pass looks_like_template through
this function to cp_parser_template_id_expr to avoid repeating
logic unnecessarily.
(cp_parser_lambda_declarator_opt): Alter function arguments of
cp_parser_skip_to_end_of_template_parameter_list.
(cp_parser_statement): Adjust for changes to saved_token_sentinel.
(cp_parser_template_id): Pass looks_like_template to avoid
repeating logic.
(cp_parser_template_id_expr): As above.
(cp_parser_explicit_template_declaration): Alter arguments of
call to cp_parser_skip_to_end_of_template_parameter_list.
(cp_parser_enclosed_template_argument_list): Same as above.
(cp_parser_skip_to_end_of_template_parameter_list): Now returns
bool depending on whether it succeeded. Can now start from a "<"
as well as a ">".
gcc/ChangeLog:
2021-09-17 Anthony Sharp <anthonysharp15@gmail.com>
* doc/invoke.texi: Documentation for Wmissing-template-keyword.
gcc/testsuite/ChangeLog:
2021-09-17 Anthony Sharp <anthonysharp15@gmail.com>
* g++.dg/cpp0x/decltype34.C: Add missing template keywords.
* g++.dg/cpp0x/variadic-mem_fn2.C: Add warning about missing
template keyword.
* g++.dg/cpp2a/typename14.C: Catch new template keyword warnings.
* g++.dg/parse/error11.C: Catch new (valid) errors in test case.
* g++.dg/parse/template26.C: Add missing template keywords.
* g++.old-deja/g++.pt/explicit81.C: Add missing template keywords.
* g++.dg/template/dependent-name15.C: New test for missing
template keywords.
---
gcc/c-family/c.opt | 4 +
gcc/cp/parser.c | 339 +++++++++++++-----
gcc/doc/invoke.texi | 19 +
gcc/testsuite/g++.dg/cpp0x/decltype34.C | 4 +-
gcc/testsuite/g++.dg/cpp0x/variadic-mem_fn2.C | 1 +
gcc/testsuite/g++.dg/cpp2a/typename14.C | 4 +
gcc/testsuite/g++.dg/parse/error11.C | 3 +-
gcc/testsuite/g++.dg/parse/template26.C | 6 +-
.../g++.dg/template/dependent-name15.C | 51 +++
.../g++.old-deja/g++.pt/explicit81.C | 4 +-
10 files changed, 341 insertions(+), 94 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/template/dependent-name15.C
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 9c151d19870..8a2d9bf5f30 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -848,6 +848,10 @@ Wmissing-requires
C++ ObjC++ Var(warn_missing_requires) Init(1) Warning
Warn about likely missing requires keyword.
+Wmissing-template-keyword
+C++ ObjC++ Var(warn_missing_template_keyword) Init(1) Warning
+Warn when the template keyword is missing after a member access token in a dependent member access expression if that member is a template.
+
Wmultistatement-macros
C ObjC C++ ObjC++ Var(warn_multistatement_macros) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
Warn about unsafe macros expanding to multiple statements used as a body of a clause such as if, else, while, switch, or for.
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 7a0b6234350..89b2260f8ea 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -1266,30 +1266,38 @@ cp_lexer_rollback_tokens (cp_lexer* lexer)
lexer->next_token = lexer->saved_tokens.pop ();
}
-/* RAII wrapper around the above functions, with sanity checking. Creating
- a variable saves tokens, which are committed when the variable is
- destroyed unless they are explicitly rolled back by calling the rollback
- member function. */
+/* RAII wrapper around the above functions, with sanity checking (the token
+ stream should be the same at the point of instantiation as it is at the
+ point of destruction).
+
+ Creating a variable saves tokens. MODE determines what happens when the
+ object is destroyed. 1 commits tokens, -1 rolls-back (default) and 0
+ does nothing. Calling rollback() will roll-back tokens and allows
+ the setting of MODE. */
struct saved_token_sentinel
{
cp_lexer *lexer;
unsigned len;
- bool commit;
- saved_token_sentinel(cp_lexer *lexer): lexer(lexer), commit(true)
+ int mode;
+ saved_token_sentinel (cp_lexer *_lexer, int _mode = -1)
+ : lexer (_lexer), mode (_mode)
{
len = lexer->saved_tokens.length ();
cp_lexer_save_tokens (lexer);
}
- void rollback ()
+ void rollback (int new_mode)
{
cp_lexer_rollback_tokens (lexer);
- commit = false;
+ mode = new_mode;
}
- ~saved_token_sentinel()
+ ~saved_token_sentinel ()
{
- if (commit)
+ if (mode == 1)
cp_lexer_commit_tokens (lexer);
+ else if (mode == -1)
+ cp_lexer_rollback_tokens (lexer);
+
gcc_assert (lexer->saved_tokens.length () == len);
}
};
@@ -2128,7 +2136,7 @@ static cp_expr cp_parser_primary_expression
static cp_expr cp_parser_id_expression
(cp_parser *, bool, bool, bool *, bool, bool);
static cp_expr cp_parser_unqualified_id
- (cp_parser *, bool, bool, bool, bool);
+ (cp_parser *, bool, bool, bool, bool, int = 0);
static tree cp_parser_nested_name_specifier_opt
(cp_parser *, bool, bool, bool, bool, bool = false);
static tree cp_parser_nested_name_specifier
@@ -2456,9 +2464,9 @@ static tree cp_parser_template_parameter
static tree cp_parser_type_parameter
(cp_parser *, bool *);
static tree cp_parser_template_id
- (cp_parser *, bool, bool, enum tag_types, bool);
+ (cp_parser *, bool, bool, enum tag_types, bool, int = 0);
static tree cp_parser_template_id_expr
- (cp_parser *, bool, bool, bool);
+ (cp_parser *, bool, bool, bool, int = 0);
static tree cp_parser_template_name
(cp_parser *, bool, bool, bool, enum tag_types, bool *);
static tree cp_parser_template_argument_list
@@ -2761,8 +2769,8 @@ static void cp_parser_skip_to_end_of_block_or_statement
(cp_parser *);
static bool cp_parser_skip_to_closing_brace
(cp_parser *);
-static void cp_parser_skip_to_end_of_template_parameter_list
- (cp_parser *);
+static bool cp_parser_skip_to_end_of_template_parameter_list
+ (cp_parser *, bool);
static void cp_parser_skip_to_pragma_eol
(cp_parser*, cp_token *);
static bool cp_parser_error_occurred
@@ -6053,6 +6061,92 @@ cp_parser_primary_expression (cp_parser *parser,
/*decltype*/false, idk);
}
+/* Check if we are looking at what "looks" like a template-id. Of course,
+ we can't technically know for sure whether it is a template-id without
+ doing lookup (although, the reason we are here is because the context
+ might be dependent and so it might not be possible to do any lookup
+ yet). Return 1 if it does look like a template-id. Return -1 if not.
+
+ Note that this is not perfect - it will generate false positives for
+ things like a.m < n > (0), where m and n are integers, for example. */
+static int
+next_token_begins_template_id (cp_parser* parser)
+{
+ cp_token* next_token = cp_lexer_peek_token (parser->lexer);
+
+ /* For performance's sake, quickly return from the most common case. */
+ if (next_token->type == CPP_NAME
+ && cp_lexer_peek_nth_token (parser->lexer, 2)->type != CPP_LESS)
+ return -1;
+
+ /* For these purposes we must believe all "template" keywords; identifiers
+ without <> at the end can still be template-ids, but they require the
+ template keyword. The template keyword is the only way we can tell those
+ kinds of ids are template-ids. */
+ 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;
+ }
+
+ /* E.g. "::operator- <>". */
+ if (next_token->keyword == RID_OPERATOR)
+ {
+ /* We could see "operator< <>" or "operator<< <>" ("<<" might have been
+ treated as two "<"s). If we see "operator<<", and it
+ was treated as two "<"s, we will assume this is a template;
+ there is no (simple) way of knowing for sure. */
+ if (cp_lexer_peek_nth_token (parser->lexer, 3)->type == CPP_LESS)
+ return 1;
+ else
+ 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;
+ }
+
+ if (next_token->type == CPP_TEMPLATE_ID)
+ return 1;
+
+ if (next_token->type != CPP_NAME
+ || cp_lexer_peek_nth_token (parser->lexer, 2)->type != CPP_LESS)
+ return -1;
+
+ saved_token_sentinel saved_tokens (parser->lexer);
+
+ /* Consume the name and the "<" because
+ cp_parser_skip_to_end_of_template_parameter_list requires it. */
+ cp_lexer_consume_token (parser->lexer);
+ cp_lexer_consume_token (parser->lexer);
+
+ /* Skip to the end. If it fails, it wasn't a template. */
+ if (!cp_parser_skip_to_end_of_template_parameter_list (parser, true))
+ return -1;
+
+ cp_token* following_token = cp_lexer_peek_token (parser->lexer);
+
+ /* If this is a function template then we would see a "(" after the
+ final ">". It could also be a class template constructor. */
+ if (following_token->type == CPP_OPEN_PAREN
+ /* If this is a class template then we would expect to see ";" or a
+ scope token after the final ">". */
+ || following_token->type == CPP_SEMICOLON
+ || following_token->type == CPP_SCOPE)
+ return 1;
+
+ return -1;
+}
+
/* Parse an id-expression.
id-expression:
@@ -6119,6 +6213,49 @@ cp_parser_id_expression (cp_parser *parser,
template_keyword_p)
!= NULL_TREE);
+ /* At this point the next token might be the template keyword. */
+ if (cp_lexer_next_token_is_keyword (parser->lexer, RID_TEMPLATE))
+ template_keyword_p = true;
+
+ /* If this doesn't look like a template-id, there is no point
+ trying to parse it as one. By checking first, we usually speed
+ compilation up, and it allows us to spot missing "template"
+ keywords. */
+ int looks_like_template_id
+ = next_token_begins_template_id (parser);
+
+ cp_token* next_token = cp_lexer_peek_token (parser->lexer);
+
+ /* If this is a dependent member-access expression accessing a template
+ member without the "template" keyword, issue a helpful warning. */
+ if (looks_like_template_id == 1
+ && !template_keyword_p
+ /* . and -> access tokens. */
+ && (dependent_scope_p (parser->context->object_type)
+ /* :: access expressions. */
+ || (dependent_scope_p (parser->scope)
+ /* A template cannot name a constructor. But don't complain if
+ missing the template keyword in that case; continue on and
+ an error for the (ill-formed) named constructor will get
+ thrown later. */
+ && !(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))
+ && !declarator_p))
+ /* ~ before a class template-id disallows the "template" keyword.
+ Probably because in the case of A::~A<>, it is certain that
+ A is a class template. Although a destructor cannot be
+ a template, you can call the destructor of a class template -
+ e.g. see "__node->~_Rb_tree_node<_Val>();" in stl_tree.h. */
+ && next_token->type != CPP_COMPL)
+ {
+ warning_at (next_token->location, OPT_Wmissing_template_keyword,
+ "expected \"template\" keyword before dependent template "
+ "name");
+ }
+
/* If there is a nested-name-specifier, then we are looking at
the first qualified-id production. */
if (nested_name_specifier_p)
@@ -6127,22 +6264,28 @@ cp_parser_id_expression (cp_parser *parser,
tree saved_object_scope;
tree saved_qualifying_scope;
cp_expr unqualified_id;
- bool is_template;
- /* See if the next token is the `template' keyword. */
- if (!template_p)
- template_p = &is_template;
- *template_p = cp_parser_optional_template_keyword (parser);
+ /* We already know if it's there or not, we just need to
+ consume it. */
+ if (template_keyword_p)
+ {
+ cp_parser_optional_template_keyword (parser);
+
+ if (template_p)
+ *template_p = true;
+ }
+
/* Name lookup we do during the processing of the
unqualified-id might obliterate SCOPE. */
saved_scope = parser->scope;
saved_object_scope = parser->object_scope;
saved_qualifying_scope = parser->qualifying_scope;
/* Process the final unqualified-id. */
- unqualified_id = cp_parser_unqualified_id (parser, *template_p,
+ unqualified_id = cp_parser_unqualified_id (parser, template_keyword_p,
check_dependency_p,
declarator_p,
- /*optional_p=*/false);
+ /*optional_p=*/false,
+ looks_like_template_id);
/* Restore the SAVED_SCOPE for our caller. */
parser->scope = saved_scope;
parser->object_scope = saved_object_scope;
@@ -6154,33 +6297,23 @@ cp_parser_id_expression (cp_parser *parser,
of the other qualified-id productions. */
else if (global_scope_p)
{
- cp_token *token;
- tree id;
-
- /* Peek at the next token. */
- token = cp_lexer_peek_token (parser->lexer);
-
- /* If it's an identifier, and the next token is not a "<", then
- we can avoid the template-id case. This is an optimization
- for this common case. */
- if (token->type == CPP_NAME
- && !cp_parser_nth_token_starts_template_argument_list_p
- (parser, 2))
- return cp_parser_identifier (parser);
-
- cp_parser_parse_tentatively (parser);
- /* Try a template-id. */
- id = cp_parser_template_id_expr (parser,
- /*template_keyword_p=*/false,
- /*check_dependency_p=*/true,
- declarator_p);
- /* If that worked, we're done. */
- if (cp_parser_parse_definitely (parser))
- return id;
+ if (looks_like_template_id == 1)
+ {
+ cp_parser_parse_tentatively (parser);
+ /* Try a template-id. */
+ tree id = cp_parser_template_id_expr (parser,
+ /*template_keyword_p=*/false,
+ /*check_dependency_p=*/true,
+ declarator_p,
+ looks_like_template_id);
+ /* If that worked, we're done. */
+ if (cp_parser_parse_definitely (parser))
+ return id;
+ }
/* Peek at the next token. (Changes in the token buffer may
have invalidated the pointer obtained above.) */
- token = cp_lexer_peek_token (parser->lexer);
+ cp_token *token = cp_lexer_peek_token (parser->lexer);
switch (token->type)
{
@@ -6201,7 +6334,8 @@ cp_parser_id_expression (cp_parser *parser,
return cp_parser_unqualified_id (parser, template_keyword_p,
/*check_dependency_p=*/true,
declarator_p,
- optional_p);
+ optional_p,
+ looks_like_template_id);
}
/* Parse an unqualified-id.
@@ -6216,6 +6350,10 @@ cp_parser_id_expression (cp_parser *parser,
If TEMPLATE_KEYWORD_P is TRUE, we have just seen the `template'
keyword, in a construct like `A::template ...'.
+ If LOOKS_LIKE_TEMPLATE_ID is 1, we checked and saw that this looks
+ like a template-id. If it is -1, we checked and saw that it did
+ not look like a template-id. If 0, we have not checked.
+
Returns a representation of unqualified-id. For the `identifier'
production, an IDENTIFIER_NODE is returned. For the `~ class-name'
production a BIT_NOT_EXPR is returned; the operand of the
@@ -6231,7 +6369,8 @@ cp_parser_unqualified_id (cp_parser* parser,
bool template_keyword_p,
bool check_dependency_p,
bool declarator_p,
- bool optional_p)
+ bool optional_p,
+ int looks_like_template_id)
{
cp_token *token;
@@ -6242,18 +6381,21 @@ cp_parser_unqualified_id (cp_parser* parser,
{
case CPP_NAME:
{
- tree id;
-
- /* We don't know yet whether or not this will be a
- template-id. */
- cp_parser_parse_tentatively (parser);
- /* Try a template-id. */
- id = cp_parser_template_id_expr (parser, template_keyword_p,
- check_dependency_p,
- declarator_p);
- /* If it worked, we're done. */
- if (cp_parser_parse_definitely (parser))
- return id;
+ if (looks_like_template_id != -1)
+ {
+ /* We don't know yet whether or not this will be a
+ template-id. */
+ cp_parser_parse_tentatively (parser);
+ /* Try a template-id. */
+ tree id = cp_parser_template_id_expr (parser,
+ template_keyword_p,
+ check_dependency_p,
+ declarator_p,
+ looks_like_template_id);
+ /* If it worked, we're done. */
+ if (cp_parser_parse_definitely (parser))
+ return id;
+ }
/* Otherwise, it's an ordinary identifier. */
return cp_parser_identifier (parser);
}
@@ -6261,7 +6403,8 @@ cp_parser_unqualified_id (cp_parser* parser,
case CPP_TEMPLATE_ID:
return cp_parser_template_id_expr (parser, template_keyword_p,
check_dependency_p,
- declarator_p);
+ declarator_p,
+ looks_like_template_id);
case CPP_COMPL:
{
@@ -11382,7 +11525,7 @@ cp_parser_lambda_declarator_opt (cp_parser* parser, tree lambda_expr)
cp_lexer_consume_token (parser->lexer);
template_param_list = cp_parser_template_parameter_list (parser);
- cp_parser_skip_to_end_of_template_parameter_list (parser);
+ cp_parser_skip_to_end_of_template_parameter_list (parser, false);
/* We may have a constrained generic lambda; parse the requires-clause
immediately after the template-parameter-list and combine with any
@@ -12059,7 +12202,7 @@ cp_parser_statement (cp_parser* parser, tree in_statement_expr,
/* There is no statement yet. */
statement = NULL_TREE;
- saved_token_sentinel saved_tokens (parser->lexer);
+ saved_token_sentinel saved_tokens (parser->lexer, 1);
token = cp_lexer_peek_token (parser->lexer);
attrs_loc = token->location;
if (c_dialect_objc ())
@@ -12193,7 +12336,7 @@ cp_parser_statement (cp_parser* parser, tree in_statement_expr,
{
/* Attributes should be parsed as part of the
declaration, so let's un-parse them. */
- saved_tokens.rollback();
+ saved_tokens.rollback (0);
std_attrs = NULL_TREE;
}
cp_parser_declaration_statement (parser);
@@ -12263,11 +12406,11 @@ cp_parser_statement (cp_parser* parser, tree in_statement_expr,
{
if (saved_tokens.lexer == lexer)
{
- if (saved_tokens.commit)
+ if (saved_tokens.mode == 1)
cp_lexer_commit_tokens (lexer);
gcc_assert (lexer->saved_tokens.length () == saved_tokens.len);
saved_tokens.lexer = parser->lexer;
- saved_tokens.commit = false;
+ saved_tokens.mode = 0;
saved_tokens.len = parser->lexer->saved_tokens.length ();
}
cp_lexer_destroy (lexer);
@@ -12298,7 +12441,7 @@ cp_parser_statement (cp_parser* parser, tree in_statement_expr,
if (has_std_attrs)
/* Attributes should be parsed as part of the declaration,
so let's un-parse them. */
- saved_tokens.rollback();
+ saved_tokens.rollback (0);
parser->omp_attrs_forbidden_p = omp_attrs_forbidden_p;
cp_parser_parse_tentatively (parser);
@@ -17985,6 +18128,11 @@ cp_parser_type_parameter (cp_parser* parser, bool *is_parameter_pack)
of functions, returns a TEMPLATE_ID_EXPR. If the template-name
names a class, returns a TYPE_DECL for the specialization.
+ If LOOKS_LIKE_TEMPLATE is 1, then we have already discovered
+ that this looks like a template-id. If -1, we have checked and seen
+ that this does not look like a template-id. If it is 0, then we
+ did not check.
+
If CHECK_DEPENDENCY_P is FALSE, names are looked up in
uninstantiated templates. */
@@ -17993,7 +18141,8 @@ cp_parser_template_id (cp_parser *parser,
bool template_keyword_p,
bool check_dependency_p,
enum tag_types tag_type,
- bool is_declaration)
+ bool is_declaration,
+ int looks_like_template_id)
{
tree templ;
tree arguments;
@@ -18014,10 +18163,11 @@ cp_parser_template_id (cp_parser *parser,
/* Avoid performing name lookup if there is no possibility of
finding a template-id. */
- if ((token->type != CPP_NAME && token->keyword != RID_OPERATOR)
+ if (looks_like_template_id == -1
+ || (token->type != CPP_NAME && token->keyword != RID_OPERATOR)
|| (token->type == CPP_NAME
&& !cp_parser_nth_token_starts_template_argument_list_p
- (parser, 2)))
+ (parser, 2)))
{
cp_parser_error (parser, "expected template-id");
return error_mark_node;
@@ -18244,10 +18394,12 @@ static tree
cp_parser_template_id_expr (cp_parser *parser,
bool template_keyword_p,
bool check_dependency_p,
- bool is_declaration)
+ bool is_declaration,
+ int looks_like_template_id)
{
tree x = cp_parser_template_id (parser, template_keyword_p, check_dependency_p,
- none_type, is_declaration);
+ none_type, is_declaration,
+ looks_like_template_id);
if (TREE_CODE (x) == TEMPLATE_ID_EXPR
&& concept_check_p (x))
/* We didn't check the arguments in cp_parser_template_id; do that now. */
@@ -31338,7 +31490,7 @@ cp_parser_explicit_template_declaration (cp_parser* parser, bool member_p)
}
/* Look for the `>'. */
- cp_parser_skip_to_end_of_template_parameter_list (parser);
+ cp_parser_skip_to_end_of_template_parameter_list (parser, false);
/* Manage template requirements */
if (flag_concepts)
@@ -31857,7 +32009,7 @@ cp_parser_enclosed_template_argument_list (cp_parser* parser)
}
}
else
- cp_parser_skip_to_end_of_template_parameter_list (parser);
+ cp_parser_skip_to_end_of_template_parameter_list (parser, false);
/* The `>' token might be a greater-than operator again now. */
parser->greater_than_is_operator_p
= saved_greater_than_is_operator_p;
@@ -32749,21 +32901,36 @@ cp_parser_require (cp_parser* parser,
}
}
-/* An error message is produced if the next token is not '>'.
- All further tokens are skipped until the desired token is
- found or '{', '}', ';' or an unbalanced ')' or ']'. */
+/* If FROM_START is false, an error message is produced if the next
+ token is not '>', and the function immediately returns if it is '>'.
+ If FROM_START is true, the next token must be the one after the
+ initial '<' of a parameter list. All further tokens are skipped
+ until the desired token is found or '{', '}', ';' or an unbalanced
+ ')' or ']'.
-static void
-cp_parser_skip_to_end_of_template_parameter_list (cp_parser* parser)
+ Returns true if we successfully reached the end, and false if
+ something unexpected happened (e.g. end of file). */
+
+static bool
+cp_parser_skip_to_end_of_template_parameter_list (cp_parser* parser,
+ bool from_start)
{
/* Current level of '< ... >'. */
unsigned level = 0;
/* Ignore '<' and '>' nested inside '( ... )' or '[ ... ]'. */
unsigned nesting_depth = 0;
- /* Are we ready, yet? If not, issue error message. */
- if (cp_parser_require (parser, CPP_GREATER, RT_GREATER))
- return;
+ if (!from_start)
+ {
+ /* Are we ready, yet? If not, issue error message. */
+ if (cp_parser_require (parser, CPP_GREATER, RT_GREATER))
+ return false;
+ }
+ else
+ {
+ if (cp_lexer_previous_token (parser->lexer)->type != CPP_LESS)
+ return false;
+ }
/* Skip tokens until the desired token is found. */
while (true)
@@ -32788,7 +32955,7 @@ cp_parser_skip_to_end_of_template_parameter_list (cp_parser* parser)
spurious. Just consume the `>>' and stop; we've
already produced at least one error. */
cp_lexer_consume_token (parser->lexer);
- return;
+ return false;
}
/* Fall through for C++0x, so we handle the second `>' in
the `>>'. */
@@ -32799,7 +32966,7 @@ cp_parser_skip_to_end_of_template_parameter_list (cp_parser* parser)
{
/* We've reached the token we want, consume it and stop. */
cp_lexer_consume_token (parser->lexer);
- return;
+ return true;
}
break;
@@ -32811,7 +32978,7 @@ cp_parser_skip_to_end_of_template_parameter_list (cp_parser* parser)
case CPP_CLOSE_PAREN:
case CPP_CLOSE_SQUARE:
if (nesting_depth-- == 0)
- return;
+ return false;
break;
case CPP_EOF:
@@ -32820,7 +32987,7 @@ cp_parser_skip_to_end_of_template_parameter_list (cp_parser* parser)
case CPP_OPEN_BRACE:
case CPP_CLOSE_BRACE:
/* The '>' was probably forgotten, don't look further. */
- return;
+ return false;
default:
break;
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 78cfc100ac2..9e5edf85de5 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -8776,6 +8776,25 @@ type @samp{T}.
This warning can be disabled with @option{-Wno-missing-requires}.
+@item -Wno-missing-template-keyword
+@opindex Wmissing-template-keyword
+@opindex Wno-missing-template-keyword
+
+The member access tokens ., -> and :: must be followed by the @code{template}
+keyword if the parent object is dependent and the member being named is a
+template.
+
+@smallexample
+template <class X>
+void DoStuff (X x)
+@{
+ x.template DoSomeOtherStuff<X>(); // Good.
+ x.DoMoreStuff<X>(); // Warning, x is dependent.
+@}
+@end smallexample
+
+This warning can be disabled with @option{-Wno-missing-template-keyword}.
+
@item -Wno-multichar
@opindex Wno-multichar
@opindex Wmultichar
diff --git a/gcc/testsuite/g++.dg/cpp0x/decltype34.C b/gcc/testsuite/g++.dg/cpp0x/decltype34.C
index 028e50669f9..b5e7a5cba0c 100644
--- a/gcc/testsuite/g++.dg/cpp0x/decltype34.C
+++ b/gcc/testsuite/g++.dg/cpp0x/decltype34.C
@@ -7,13 +7,13 @@ struct impl
};
template<class T, class U,
- class = decltype(impl::create<T>()->impl::create<U>())>
+ class = decltype(impl::create<T>()->impl::template create<U>())>
struct tester{};
tester<impl*, int> ti;
template<class T, class U,
- class = decltype(impl::create<T>()->impl::create<U>())>
+ class = decltype(impl::create<T>()->impl::template create<U>())>
int test() { return 0; }
int i = test<impl*, int>();
diff --git a/gcc/testsuite/g++.dg/cpp0x/variadic-mem_fn2.C b/gcc/testsuite/g++.dg/cpp0x/variadic-mem_fn2.C
index 4a02ab22990..c343f32086d 100644
--- a/gcc/testsuite/g++.dg/cpp0x/variadic-mem_fn2.C
+++ b/gcc/testsuite/g++.dg/cpp0x/variadic-mem_fn2.C
@@ -5,5 +5,6 @@ template <class A0, class... As> struct tuple
tuple<As...> tail;
template <int Offset, class... More> int apply(const More&... more) {
return tail.apply<1>(more...); // { dg-error "" } needs .template
+ // { dg-warning "keyword before dependent template name" "" { target *-*-* } .-1 }
}
};
diff --git a/gcc/testsuite/g++.dg/cpp2a/typename14.C b/gcc/testsuite/g++.dg/cpp2a/typename14.C
index ba7dad8245f..ede28be75dc 100644
--- a/gcc/testsuite/g++.dg/cpp2a/typename14.C
+++ b/gcc/testsuite/g++.dg/cpp2a/typename14.C
@@ -9,6 +9,8 @@ template<typename> struct A
template<typename T>
template<typename U>
A<T>::A<U> () // { dg-error "" }
+// { 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.
{
}
@@ -20,6 +22,8 @@ template<typename> struct B
template<typename T>
template<typename U>
B<T>::foo<int>(int) // { dg-error "" }
+// { 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.
{
return 1;
}
diff --git a/gcc/testsuite/g++.dg/parse/error11.C b/gcc/testsuite/g++.dg/parse/error11.C
index 4baf97e531d..3b2b56ad6d4 100644
--- a/gcc/testsuite/g++.dg/parse/error11.C
+++ b/gcc/testsuite/g++.dg/parse/error11.C
@@ -20,9 +20,10 @@ struct Foo
// { dg-message "17:'<:' is an alternate spelling" "17-alt" { target { ! c++11 } } .-1 }
// { dg-error "39:'<::' cannot begin" "39-begin" { target { ! c++11 } } .-2 }
// { dg-message "39:'<:' is an alternate spelling" "39-alt" { target { ! c++11 } } .-3 }
- n.template Nested<B>::method();
+ n.template Nested<B>::method(); // { dg-error "is not a template" { target *-*-* } }
n.template Nested<::B>::method(); // { dg-error "22:'<::' cannot begin" "error" { target { ! c++11 } } }
// { dg-message "22:'<:' is an alternate" "note" { target { ! c++11 } } .-1 }
+// { dg-error "is not a template" { target *-*-* } .-2 }
Nested<B>::method();
Nested<::B>::method(); // { dg-error "11:'<::' cannot begin" "error" { target { ! c++11 } } }
// { dg-message "11:'<:' is an alternate" "note" { target { ! c++11 } } .-1 }
diff --git a/gcc/testsuite/g++.dg/parse/template26.C b/gcc/testsuite/g++.dg/parse/template26.C
index aab9763ccaf..add8c64eabb 100644
--- a/gcc/testsuite/g++.dg/parse/template26.C
+++ b/gcc/testsuite/g++.dg/parse/template26.C
@@ -6,13 +6,13 @@ namespace impl
}
template <class T, class U, __SIZE_TYPE__
- = sizeof(impl::create<T>()->*impl::create<U>())>
+ = sizeof(impl::create<T>()->*impl::template create<U>())>
struct foo1;
template <class T, class U, __SIZE_TYPE__
- = sizeof(impl::create<T>()->impl::create<U>())> // { dg-error "not a class member" }
+ = sizeof(impl::create<T>()->impl::template create<U>())> // { dg-error "not a class member" }
struct foo2;
template <class T, class U, __SIZE_TYPE__
- = sizeof(impl::create<T>().impl::create<U>())> // { dg-error "not a class member" }
+ = sizeof(impl::create<T>().impl::template create<U>())> // { dg-error "not a class member" }
struct foo3;
diff --git a/gcc/testsuite/g++.dg/template/dependent-name15.C b/gcc/testsuite/g++.dg/template/dependent-name15.C
new file mode 100644
index 00000000000..ecf4c89756f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/dependent-name15.C
@@ -0,0 +1,51 @@
+// C++ PR 70317
+// { dg-do compile }
+
+template<class T> class mytemplateclass
+{
+ public:
+ template<class U> void class_func() {}
+ template<class U> static void class_func_static() {}
+};
+
+class myclass
+{
+ public:
+ int testint;
+ template<class U> void class_func() {}
+ template<class U> static void class_func_static() {}
+};
+
+template<class Y> void tests_func(mytemplateclass<Y> c, myclass c2)
+{
+ /* Dependent template accessors (ill-formed code). */
+ c.class_func<Y>(); // { dg-warning "keyword before dependent template name" }
+ // { dg-error "expected primary-expression" "" { target *-*-* } .-1 }
+ (&c)->class_func<Y>(); // { dg-warning "keyword before dependent template name" }
+ // { dg-error "expected primary-expression" "" { target *-*-* } .-1 }
+ mytemplateclass<Y>::class_func_static<Y>(); // { dg-warning "keyword before dependent template name" }
+ // { dg-error "expected primary-expression" "" { target *-*-* } .-1 }
+
+ /* Dependent template accessors (well-formed code). */
+ c.template class_func<Y>();
+ (&c)->template class_func<Y>();
+ mytemplateclass<Y>::template class_func_static<Y>();
+
+ /* Non-dependent template accessors (well-formed code). */
+ c2.class_func<myclass>();
+ (&c2)->class_func<myclass>();
+ myclass::class_func_static<myclass>();
+}
+
+int main()
+{
+ mytemplateclass<myclass> c;
+ myclass c2;
+ tests_func<myclass>(c, c2);
+
+ c2.testint = 53;
+ /* Make sure this isn't treated as a template. */
+ bool testbool = c2.testint < 999 > 7;
+ /* This probably will be treated as a template initially but it should still work. */
+ testbool = c2.testint < 123 > (50);
+}
\ No newline at end of file
diff --git a/gcc/testsuite/g++.old-deja/g++.pt/explicit81.C b/gcc/testsuite/g++.old-deja/g++.pt/explicit81.C
index 576ba5439a6..5232d770720 100644
--- a/gcc/testsuite/g++.old-deja/g++.pt/explicit81.C
+++ b/gcc/testsuite/g++.old-deja/g++.pt/explicit81.C
@@ -26,10 +26,10 @@ void tf(C *ptr)
{
N::nf<N::e0>();
gf<N::e0>();
- ptr->X::xfn <N::e0> ();
+ ptr->X::template xfn <N::e0> ();
ptr->C::template xfn <N::e0> ();
ptr->template xfn <N::e0> ();
- ptr->X::sfn <N::e0> ();
+ ptr->X::template sfn <N::e0> ();
ptr->C::template sfn <N::e0> ();
ptr->template sfn <N::e0> ();
X::sfn <N::e0> ();
--
2.33.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] c++: error message for dependent template members [PR70417]
2021-09-17 22:22 ` Anthony Sharp
@ 2021-09-22 20:04 ` Jason Merrill
2021-10-10 11:28 ` Anthony Sharp
0 siblings, 1 reply; 12+ messages in thread
From: Jason Merrill @ 2021-09-22 20:04 UTC (permalink / raw)
To: Anthony Sharp, gcc-patches
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 <anthonysharp15@gmail.com> wrote:
>>
>> Re-adding gcc-patches@gcc.gnu.org.
>>
>> ---------- Forwarded message ---------
>> From: Anthony Sharp <anthonysharp15@gmail.com>
>> Date: Fri, 17 Sept 2021 at 23:11
>> Subject: Re: [PATCH] c++: error message for dependent template members [PR70417]
>> To: Jason Merrill <jason@redhat.com>
>>
>>
>> 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<U> 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 <class T, int N> void f(T t) { t.m<N>(0); }
>>> struct A { int m; } a;
>>> int main() { f<A,42>(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,
>> 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.
>>
>> Thanks for the help,
>> Anthony
> +/* Check if we are looking at what "looks" like a template-id. Of course,
> + we can't technically know for sure whether it is a template-id without
> + doing lookup (although, the reason we are here is because the context
> + might be dependent and so it might not be possible to do any lookup
> + yet). Return 1 if it does look like a template-id. Return -1 if not.
> +
> + Note that this is not perfect - it will generate false positives for
> + things like a.m < n > (0), where m and n are integers, for example. */
> +static int
> +next_token_begins_template_id (cp_parser* parser)
> +{
> + cp_token* next_token = cp_lexer_peek_token (parser->lexer);
> +
> + /* For performance's sake, quickly return from the most common case. */
> + if (next_token->type == CPP_NAME
> + && cp_lexer_peek_nth_token (parser->lexer, 2)->type != CPP_LESS)
Let's use cp_parser_nth_token_starts_template_argument_list_p.
> + return -1;
> +
> + /* For these purposes we must believe all "template" keywords; identifiers
> + without <> at the end can still be template-ids, but they require the
> + template keyword. The template keyword is the only way we can tell those
> + kinds of ids are template-ids. */
Without the <> they may be template names, but they aren't template-ids.
> + 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;
> + }
> +
> + /* E.g. "::operator- <>". */
> + if (next_token->keyword == RID_OPERATOR)
> + {
> + /* We could see "operator< <>" or "operator<< <>" ("<<" might have been
> + treated as two "<"s). If we see "operator<<", and it
> + was treated as two "<"s, we will assume this is a template;
> + there is no (simple) way of knowing for sure. */
I don't think << is ever treated as two <; that only applies to >>.
> + if (cp_lexer_peek_nth_token (parser->lexer, 3)->type == CPP_LESS)
> + return 1;
I think we don't want to return early here, we want to go through the
same <args>( check that we do for regular names.
> + else
> + 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;
> + }
> +
> + if (next_token->type == CPP_TEMPLATE_ID)
> + return 1;
> +
> + if (next_token->type != CPP_NAME
> + || cp_lexer_peek_nth_token (parser->lexer, 2)->type != CPP_LESS)
> + return -1;
The || case is already handled by the test at the top of the function,
right?
> + saved_token_sentinel saved_tokens (parser->lexer);
> + /* Consume the name and the "<" because
> + cp_parser_skip_to_end_of_template_parameter_list requires it. */
> + cp_lexer_consume_token (parser->lexer);
> + cp_lexer_consume_token (parser->lexer);
> +
> + /* Skip to the end. If it fails, it wasn't a template. */
> + if (!cp_parser_skip_to_end_of_template_parameter_list (parser, true))
> + return -1;
How about factoring this call and the previous line into a
cp_parser_skip_template_argument_list (parser)
?
> - /* Are we ready, yet? If not, issue error message. */
> - if (cp_parser_require (parser, CPP_GREATER, RT_GREATER))
> - return;
We could also factor this check into a separate function, leaving only
the actual walking that's shared between the two callers.
> + cp_token* following_token = cp_lexer_peek_token (parser->lexer);
> +
> + /* If this is a function template then we would see a "(" after the
> + final ">". It could also be a class template constructor. */
> + if (following_token->type == CPP_OPEN_PAREN
> + /* If this is a class template then we would expect to see ";" or a
> + scope token after the final ">". */
I'm not sure how you'd see ';' after a class template-id, but you could
see it for a variable template. Along with many other possible tokens.
I guess any operator-or-punctuator after the > would suggest a
template-id.
> + && constructor_name_p (cp_expr (next_token->u.value,
> + next_token->location),
You're building a cp_expr that immediately converts back to a tree; just
pass next_token->u.value to constructor_name_p.
> +@item -Wno-missing-template-keyword
> +@opindex Wmissing-template-keyword
> +@opindex Wno-missing-template-keyword
> +
> +The member access tokens ., -> and :: must be followed by the @code{template}
> +keyword if the parent object is dependent and the member being named is a
> +template.
> +
> +@smallexample
> +template <class X>
> +void DoStuff (X x)
> +@{
> + x.template DoSomeOtherStuff<X>(); // Good.
> + x.DoMoreStuff<X>(); // Warning, x is dependent.
> +@}
> +@end smallexample
> +
> +This warning can be disabled with @option{-Wno-missing-template-keyword}.
This should mention the possible false positive and how to silence it:
If you get actually meant to write two comparisons in t.m<N>(0), you can
make that clear by writing (t.m<N)>(0).
> +// { 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?
Jason
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] c++: error message for dependent template members [PR70417]
2021-09-22 20:04 ` Jason Merrill
@ 2021-10-10 11:28 ` Anthony Sharp
2021-10-19 19:15 ` Jason Merrill
0 siblings, 1 reply; 12+ messages in thread
From: Anthony Sharp @ 2021-10-10 11:28 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 17813 bytes --]
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. Hard to tell exactly where in the file
it needed to go but I took an educated guess and put it in what I think is
the parser.c bit.
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.
>
I guess it just seemed odd that with a name like
cp_parser_skip_to_end_of_template_parameter_list, the calling function
would expect that it should already be at the end of a template parameter
list. Maybe if it was called
cp_parser_check_reached_end_of_template_parameter_list... but like you
suggested that's been refactored now anyways.
Let's use cp_parser_nth_token_starts_template_argument_list_p.
>
Good point. Surprised that didn't trigger a failed test, but maybe there
just isn't one.
Without the <> they may be template names, but they aren't template-ids.
>
Very true, updated the comment to hopefully make more sense.
I don't think << is ever treated as two <; that only applies to >>.
>
Good to know, I've taken that comment out.
I think we don't want to return early here, we want to go through the
> same <args>( 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?
The || case is already handled by the test at the top of the function,
> right?
>
I thought this too originally but I don't think it is. The first check is
just a performance booster, and only returns if it sees a name without a <
after it. If it doesn't see a name, that's fine, because it might instead
be a keyword like operator or template, so we continue. We then checked for
the template and keyword operators and returned appropriately if we saw one
(although, now, because of the change above, we continue if we see the
operator keyword). But at this point we haven't checked what comes after
the operator keyword. It could be gibberish. So at that point we ensure
that we see a name and a < at that point. I don't think this check can go
any earlier, since that's the earliest we can safely ensure we see both a
name and a <, given the possibility of finding keywords initially. Although
that whole part had to be refactored so it might be clearer now anyways.
> + /* Consume the name and the "<" because
> + cp_parser_skip_to_end_of_template_parameter_list requires it. */
> + cp_lexer_consume_token (parser->lexer);
> + cp_lexer_consume_token (parser->lexer);
> +
> + /* Skip to the end. If it fails, it wasn't a template. */
> + if (!cp_parser_skip_to_end_of_template_parameter_list (parser, true))
> + return -1;
How about factoring this call and the previous line into a
cp_parser_skip_template_argument_list (parser)
?
> - /* Are we ready, yet? If not, issue error message. */
> - if (cp_parser_require (parser, CPP_GREATER, RT_GREATER))
> - return;
We could also factor this check into a separate function, leaving only
the actual walking that's shared between the two callers.
Yeah I think that would make things a lot clearer. I think I've done what
you've suggested - 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.
I've then made another function called
cp_parser_skip_entire_template_parameter_list that consumes the "<" and
then skips to the end by calling the other function.
I'm not sure how you'd see ';' after a class template-id, but you could
> see it for a variable template. Along with many other possible tokens.
> I guess any operator-or-punctuator after the > would suggest a
> template-id.
>
At least one of us knows what they're talking about. Updated the comment.
You're building a cp_expr that immediately converts back to a tree; just
> pass next_token->u.value to constructor_name_p.
>
Oops... fixed.
This should mention the possible false positive and how to silence it:
> If you get actually meant to write two comparisons in t.m<N>(0), you can
> make that clear by writing (t.m<N)>(0).
>
Done.
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.
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 <jason@redhat.com> 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 <anthonysharp15@gmail.com>
> wrote:
> >>
> >> Re-adding gcc-patches@gcc.gnu.org.
> >>
> >> ---------- Forwarded message ---------
> >> From: Anthony Sharp <anthonysharp15@gmail.com>
> >> Date: Fri, 17 Sept 2021 at 23:11
> >> Subject: Re: [PATCH] c++: error message for dependent template members
> [PR70417]
> >> To: Jason Merrill <jason@redhat.com>
> >>
> >>
> >> 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<U> 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 <class T, int N> void f(T t) { t.m<N>(0); }
> >>> struct A { int m; } a;
> >>> int main() { f<A,42>(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,
> >>
> 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.
> >>
> >> Thanks for the help,
> >> Anthony
>
> > +/* Check if we are looking at what "looks" like a template-id. Of
> course,
> > + we can't technically know for sure whether it is a template-id
> without
> > + doing lookup (although, the reason we are here is because the context
> > + might be dependent and so it might not be possible to do any lookup
> > + yet). Return 1 if it does look like a template-id. Return -1 if
> not.
> > +
> > + Note that this is not perfect - it will generate false positives for
> > + things like a.m < n > (0), where m and n are integers, for example.
> */
> > +static int
> > +next_token_begins_template_id (cp_parser* parser)
> > +{
> > + cp_token* next_token = cp_lexer_peek_token (parser->lexer);
> > +
> > + /* For performance's sake, quickly return from the most common case.
> */
> > + if (next_token->type == CPP_NAME
> > + && cp_lexer_peek_nth_token (parser->lexer, 2)->type != CPP_LESS)
>
> Let's use cp_parser_nth_token_starts_template_argument_list_p.
>
> > + return -1;
> > +
> > + /* For these purposes we must believe all "template" keywords;
> identifiers
> > + without <> at the end can still be template-ids, but they require
> the
> > + template keyword. The template keyword is the only way we can
> tell those
> > + kinds of ids are template-ids. */
>
> Without the <> they may be template names, but they aren't template-ids.
>
> > + 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;
> > + }
> > +
> > + /* E.g. "::operator- <>". */
> > + if (next_token->keyword == RID_OPERATOR)
> > + {
> > + /* We could see "operator< <>" or "operator<< <>" ("<<" might
> have been
> > + treated as two "<"s). If we see "operator<<", and it
> > + was treated as two "<"s, we will assume this is a template;
> > + there is no (simple) way of knowing for sure. */
>
> I don't think << is ever treated as two <; that only applies to >>.
>
> > + if (cp_lexer_peek_nth_token (parser->lexer, 3)->type == CPP_LESS)
> > + return 1;
>
> I think we don't want to return early here, we want to go through the
> same <args>( check that we do for regular names.
>
> > + else
> > + 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;
> > + }
> > +
> > + if (next_token->type == CPP_TEMPLATE_ID)
> > + return 1;
> > +
> > + if (next_token->type != CPP_NAME
> > + || cp_lexer_peek_nth_token (parser->lexer, 2)->type != CPP_LESS)
> > + return -1;
>
> The || case is already handled by the test at the top of the function,
> right?
>
> > + saved_token_sentinel saved_tokens (parser->lexer);
>
> > + /* Consume the name and the "<" because
> > + cp_parser_skip_to_end_of_template_parameter_list requires it. */
> > + cp_lexer_consume_token (parser->lexer);
> > + cp_lexer_consume_token (parser->lexer);
> > +
> > + /* Skip to the end. If it fails, it wasn't a template. */
> > + if (!cp_parser_skip_to_end_of_template_parameter_list (parser, true))
> > + return -1;
>
> How about factoring this call and the previous line into a
>
> cp_parser_skip_template_argument_list (parser)
>
> ?
>
> > - /* Are we ready, yet? If not, issue error message. */
> > - if (cp_parser_require (parser, CPP_GREATER, RT_GREATER))
> > - return;
>
> We could also factor this check into a separate function, leaving only
> the actual walking that's shared between the two callers.
>
> > + cp_token* following_token = cp_lexer_peek_token (parser->lexer);
> > +
> > + /* If this is a function template then we would see a "(" after the
> > + final ">". It could also be a class template constructor. */
> > + if (following_token->type == CPP_OPEN_PAREN
> > + /* If this is a class template then we would expect to see ";" or
> a
> > + scope token after the final ">". */
>
> I'm not sure how you'd see ';' after a class template-id, but you could
> see it for a variable template. Along with many other possible tokens.
> I guess any operator-or-punctuator after the > would suggest a
> template-id.
>
> > + && constructor_name_p (cp_expr (next_token->u.value,
> > + next_token->location),
>
> You're building a cp_expr that immediately converts back to a tree; just
> pass next_token->u.value to constructor_name_p.
>
> > +@item -Wno-missing-template-keyword
> > +@opindex Wmissing-template-keyword
> > +@opindex Wno-missing-template-keyword
> > +
> > +The member access tokens ., -> and :: must be followed by the
> @code{template}
> > +keyword if the parent object is dependent and the member being named is
> a
> > +template.
> > +
> > +@smallexample
> > +template <class X>
> > +void DoStuff (X x)
> > +@{
> > + x.template DoSomeOtherStuff<X>(); // Good.
> > + x.DoMoreStuff<X>(); // Warning, x is dependent.
> > +@}
> > +@end smallexample
> > +
> > +This warning can be disabled with
> @option{-Wno-missing-template-keyword}.
>
> This should mention the possible false positive and how to silence it:
> If you get actually meant to write two comparisons in t.m<N>(0), you can
> make that clear by writing (t.m<N)>(0).
>
> > +// { 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?
>
> Jason
>
>
[-- Attachment #2: pr70417_9.patch --]
[-- Type: application/x-patch, Size: 35899 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] c++: error message for dependent template members [PR70417]
2021-10-10 11:28 ` Anthony Sharp
@ 2021-10-19 19:15 ` Jason Merrill
2021-12-04 17:23 ` Anthony Sharp
0 siblings, 1 reply; 12+ messages in thread
From: Jason Merrill @ 2021-10-19 19:15 UTC (permalink / raw)
To: Anthony Sharp; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 7232 bytes --]
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 <args>( 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<t> (::operator- <>)( const vect<t>&, const vect<t>& );
> + */
> + || 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 <class T> struct A
{
template <class U> static U m;
};
template <class T> int f (T t)
{
return t.m<int> + 42;
}
This is especially a problem since we use the return value of -1 to skip
calling cp_parser_template_id_expr.
> +++ b/gcc/testsuite/g++.dg/cpp0x/decltype34.C
> @@ -7,13 +7,13 @@ struct impl
> };
>
> template<class T, class U,
> - class = decltype(impl::create<T>()->impl::create<U>())>
> + class = decltype(impl::create<T>()->impl::template create<U>())>
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.
> + int looks_like_template_id
> + = next_token_begins_template_id (parser);
We probably want this to just be 0, and not call the function, if
!warn_missing_template_keyword.
> + if (template_p)
> + *template_p = true;
Since we unconditionally dereference template_p a few lines below, let's
try removing this condition.
> + if (looks_like_template_id == 1)
> + {
Shouldn't this be != -1, as in cp_parser_unqualified_id?
> -Foo<T>::operator Foo<U>() {
> +Foo<T>::template operator Foo<U>() {
This is wrong; you don't use the template keyword like this in a declarator.
> - n.template Nested<B>::method();
> + n.template Nested<B>::method(); // { dg-error "is not a template" "" { target { *-*-* } } }
This is wrong; Nested is indeed a template.
> - = sizeof(impl::create<T>()->*impl::create<U>())>
> + = sizeof(impl::create<T>()->*impl::template create<U>())>
As above, this shouldn't be necessary currently.
> - ptr->X::xfn <N::e0> ();
> + ptr->X::template xfn <N::e0> ();
Or this.
Jason
[-- Attachment #2: 0001-c-tweak-parsing-of-invalid-types.patch --]
[-- Type: text/x-patch, Size: 2805 bytes --]
From ecccf8103b2368564d073d4b85efd9484448a59f Mon Sep 17 00:00:00 2001
From: Jason Merrill <jason@redhat.com>
Date: Mon, 18 Oct 2021 16:12:15 -0400
Subject: [PATCH] c++: tweak parsing of invalid types
To: gcc-patches@gcc.gnu.org
cp_parser_parse_and_diagnose_invalid_type_name is called during declaration
parsing, so it should pass 'true' for the declarator_p argument. But that
caused a diagnostic regression on template/pr84789.C due to undesired lookup
in dependent scopes. To fix that, cp_parser_nested_name_specifier_opt needs
to respect the value of check_dependency_p.
gcc/cp/ChangeLog:
* parser.c (cp_parser_parse_and_diagnose_invalid_type_name):
Pass true for declarator_p.
(cp_parser_nested_name_specifier_opt): Only look through
TYPENAME_TYPE if check_dependency_p is false.
---
gcc/cp/parser.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 865778e4d30..f11917259f7 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -3693,7 +3693,7 @@ cp_parser_parse_and_diagnose_invalid_type_name (cp_parser *parser)
/*template_keyword_p=*/false,
/*check_dependency_p=*/true,
/*template_p=*/NULL,
- /*declarator_p=*/false,
+ /*declarator_p=*/true,
/*optional_p=*/false);
/* If the next token is a (, this is a function with no explicit return
type, i.e. constructor, destructor or conversion op. */
@@ -6605,6 +6605,8 @@ check_template_keyword_in_nested_name_spec (tree name)
it unchanged if there is no nested-name-specifier. Returns the new
scope iff there is a nested-name-specifier, or NULL_TREE otherwise.
+ If CHECK_DEPENDENCY_P is FALSE, names are looked up in dependent scopes.
+
If IS_DECLARATION is TRUE, the nested-name-specifier is known to be
part of a declaration and/or decl-specifier. */
@@ -6645,9 +6647,10 @@ cp_parser_nested_name_specifier_opt (cp_parser *parser,
/* Grab the nested-name-specifier and continue the loop. */
cp_parser_pre_parsed_nested_name_specifier (parser);
/* If we originally encountered this nested-name-specifier
- with IS_DECLARATION set to false, we will not have
+ with CHECK_DEPENDENCY_P set to true, we will not have
resolved TYPENAME_TYPEs, so we must do so here. */
if (is_declaration
+ && !check_dependency_p
&& TREE_CODE (parser->scope) == TYPENAME_TYPE)
{
new_scope = resolve_typename_type (parser->scope,
@@ -6729,6 +6732,7 @@ cp_parser_nested_name_specifier_opt (cp_parser *parser,
a template. So, if we have a typename at this point, we make
an effort to look through it. */
if (is_declaration
+ && !check_dependency_p
&& !typename_keyword_p
&& parser->scope
&& TREE_CODE (parser->scope) == TYPENAME_TYPE)
--
2.27.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] c++: error message for dependent template members [PR70417]
2021-10-19 19:15 ` Jason Merrill
@ 2021-12-04 17:23 ` Anthony Sharp
2021-12-09 15:51 ` Jason Merrill
0 siblings, 1 reply; 12+ messages in thread
From: Anthony Sharp @ 2021-12-04 17:23 UTC (permalink / raw)
To: Jason Merrill, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 16406 bytes --]
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<t> (::operator- <>)( const vect<t>&, const vect<t>&
);
>> + */
>> + || 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 <class T> struct A
>{
> template <class U> static U m;
>};
>
>template <class T> int f (T t)
>{
> return t.m<int> + 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 T, class U,
>> - class = decltype(impl::create<T>()->impl::create<U>())>
>> + class = decltype(impl::create<T>()->impl::template create<U>())>
>
>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 more sense what it's trying to do now.
That's just my lack of experience in reading complex C++ code. Thanks.
>> + int looks_like_template_id
>> + = next_token_begins_template_id (parser);
>
>We probably want this to just be 0, and not call the function, if
>!warn_missing_template_keyword.
Added the check to warn_missing_template_keyword. We'd probably still want
to
call the function either way since it's supposed to speed things up a
little. But we can disable
the warning.
In either case, were next_token_begins_template_id not called, the way it's
set-up currently
I think we'd have to default it to 1, otherwise it will cause it to skip
some of the potentially
necessary parsing logic.
>> + if (looks_like_template_id == 1)
>> + {
>
>Shouldn't this be != -1, as in cp_parser_unqualified_id?
looks_like_template_id will always be 1 or -1 at that point (in
cp_parser_id_expression), so functionally speaking != -1 and == 1 should
hopefully give us the same result there.
The only reason we use != -1 in cp_parser_unqualified_id is because that
function
is called from many places, and so sometimes looks_like_template_id may be
0 there.
So we'd want to check the logic if we saw a 0 or a 1 there, hence the != -1.
But either way is probably just as valid.
>> -Foo<T>::operator Foo<U>() {
>> +Foo<T>::template operator Foo<U>() {
>
>This is wrong; you don't use the template keyword like this in a
declarator.
Had a moment of weakness, that's obviously wrong as you say.
>> - = sizeof(impl::create<T>()->*impl::create<U>())>
>> + = sizeof(impl::create<T>()->*impl::template create<U>())>
>
>As above, this shouldn't be necessary currently.
>
>> - ptr->X::xfn <N::e0> ();
>> + ptr->X::template xfn <N::e0> ();
>
>Or this.
Fixed as discussed.
>> + if (template_p)
>> + *template_p = true;
>
>Since we unconditionally dereference template_p a few lines below, let's
>try removing this condition.
I think I see where you mean. I had tried to refactor these parts but it
turns
out it's trickier than it seems, and was causing a few bugs. The code is a
bit
tangled at the moment; for example, assigning *template_p in circumstances
other than in the strict ways the calling functions are anticipating will
cause issues,
even if merely doing so for the purpose of indicating a found template
keyword (as the
comments above the function declaration hint at). So I've gone and put most
of that
stuff back as it was. Certainly doable to refactor that part, but probably
best left to someone
who can get more to grips with the surrounding context.
>> - n.template Nested<B>::method();
>> + n.template Nested<B>::method(); // { dg-error "is not a template"
"" { target { *-*-* } } }
>
>This is wrong; Nested is indeed a template.
Yes, sorry, this was a bug I caused that I've now fixed. Not sure why I
thought
it was a good idea to try and suppress it...
Bootstraps fine. Re-tested. g++ and gcc tests were perfect. However, for
some reason the libstdc++, libgomp and libitm tests were not running
automatically with this patch with --enable-languages=c,c++ and then a
vanilla make and make check. I had to run them manually with make
check-target-libstdc++-v3; make check-target-libitm-c++; make
check-target-libgomp-c++. And once I had done that it looked like in my
patch the number of successful tests went down and the number of
unsupported tests increased in both libstdc++ and libgomp (although
unexpected failures/successes did not change). Not sure why this was, so
might be worth bearing in mind. (Maybe my source was more/less up to date
with git than it was when I ran the original tests?)
Thanks!
Anthony
On Tue, 19 Oct 2021 at 20:15, Jason Merrill <jason@redhat.com> wrote:
> 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 <args>( 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<t> (::operator- <>)( const vect<t>&, const vect<t>&
> );
> > + */
> > + || 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 <class T> struct A
> {
> template <class U> static U m;
> };
>
> template <class T> int f (T t)
> {
> return t.m<int> + 42;
> }
>
> This is especially a problem since we use the return value of -1 to skip
> calling cp_parser_template_id_expr.
>
> > +++ b/gcc/testsuite/g++.dg/cpp0x/decltype34.C
> > @@ -7,13 +7,13 @@ struct impl
> > };
> >
> > template<class T, class U,
> > - class = decltype(impl::create<T>()->impl::create<U>())>
> > + class = decltype(impl::create<T>()->impl::template create<U>())>
>
> 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.
>
> > + int looks_like_template_id
> > + = next_token_begins_template_id (parser);
>
> We probably want this to just be 0, and not call the function, if
> !warn_missing_template_keyword.
>
> > + if (template_p)
> > + *template_p = true;
>
> Since we unconditionally dereference template_p a few lines below, let's
> try removing this condition.
>
> > + if (looks_like_template_id == 1)
> > + {
>
> Shouldn't this be != -1, as in cp_parser_unqualified_id?
>
> > -Foo<T>::operator Foo<U>() {
> > +Foo<T>::template operator Foo<U>() {
>
> This is wrong; you don't use the template keyword like this in a
> declarator.
>
> > - n.template Nested<B>::method();
> > + n.template Nested<B>::method(); // { dg-error "is not a template"
> "" { target { *-*-* } } }
>
> This is wrong; Nested is indeed a template.
>
> > - = sizeof(impl::create<T>()->*impl::create<U>())>
> > + = sizeof(impl::create<T>()->*impl::template create<U>())>
>
> As above, this shouldn't be necessary currently.
>
> > - ptr->X::xfn <N::e0> ();
> > + ptr->X::template xfn <N::e0> ();
>
> Or this.
>
> Jason
[-- Attachment #2: pr70417_10.patch --]
[-- Type: application/x-patch, Size: 27957 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] c++: error message for dependent template members [PR70417]
2021-12-04 17:23 ` Anthony Sharp
@ 2021-12-09 15:51 ` Jason Merrill
2022-01-13 21:01 ` Jason Merrill
0 siblings, 1 reply; 12+ messages in thread
From: Jason Merrill @ 2021-12-09 15:51 UTC (permalink / raw)
To: Anthony Sharp, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 3014 bytes --]
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 <class T> struct A
{
template <class U> void f(U u);
};
template <class T> void g(A<T> 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 <bool B> struct A { };
template <class T> void f()
{
A<T::I < T::J>();
};
which my patch checks in_template_argument_list_p to avoid, though
checking any_object_scope also currently avoids it.
What do you think?
Jason
[-- Attachment #2: 0001-handle-in-cp_parser_template_name.patch --]
[-- Type: text/x-patch, Size: 2513 bytes --]
From 3022a600514f8adf8706fd286387a5f9f34c06ba Mon Sep 17 00:00:00 2001
From: Jason Merrill <jason@redhat.com>
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_peek_token (parser->lexer)->type
+ <= CPP_LAST_PUNCTUATOR))
+ {
+ warning_at (token->location, OPT_Wmissing_template_keyword,
+ "expected %qs keyword before dependent "
+ "template name", "template");
+ token->error_reported = true;
+ }
+ }
+ }
if (!found)
{
--
2.27.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] c++: error message for dependent template members [PR70417]
2021-12-09 15:51 ` Jason Merrill
@ 2022-01-13 21:01 ` Jason Merrill
2022-01-15 8:27 ` Anthony Sharp
0 siblings, 1 reply; 12+ messages in thread
From: Jason Merrill @ 2022-01-13 21:01 UTC (permalink / raw)
To: Anthony Sharp, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 3417 bytes --]
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 <class T> struct A
> {
> template <class U> void f(U u);
> };
>
> template <class T> void g(A<T> 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 <bool B> struct A { };
> template <class T> void f()
> {
> A<T::I < T::J>();
> };
>
> 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!
[-- Attachment #2: 0001-c-warning-for-dependent-template-members-PR70417.patch --]
[-- Type: text/x-patch, Size: 18812 bytes --]
From 1978f05716133b934de0fca7c3d64089b62e3e78 Mon Sep 17 00:00:00 2001
From: Anthony Sharp <anthonysharp15@gmail.com>
Date: Sat, 4 Dec 2021 17:23:22 +0000
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 <jason@redhat.com>
---
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/testsuite/g++.dg/template/dependent-name17.C
create mode 100644 gcc/testsuite/g++.dg/template/dependent-name18.C
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 6b84228f142..5504971ea81 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -8963,6 +8963,39 @@ type @samp{T}.
This warning can be disabled with @option{-Wno-missing-requires}.
+@item -Wno-missing-template-keyword
+@opindex Wmissing-template-keyword
+@opindex Wno-missing-template-keyword
+
+The member access tokens ., -> and :: must be followed by the @code{template}
+keyword if the parent object is dependent and the member being named is a
+template.
+
+@smallexample
+template <class X>
+void DoStuff (X x)
+@{
+ x.template DoSomeOtherStuff<X>(); // Good.
+ x.DoMoreStuff<X>(); // Warning, x is dependent.
+@}
+@end smallexample
+
+In rare cases it is possible to get false positives. To silence this, wrap
+the expression in parentheses. For example, the following is treated as a
+template, even where m and N are integers:
+
+@smallexample
+void NotATemplate (my_class t)
+@{
+ int N = 5;
+
+ bool test = t.m < N > (0); // Treated as a template.
+ test = (t.m < N) > (0); // Same meaning, but not treated as a template.
+@}
+@end smallexample
+
+This warning can be disabled with @option{-Wno-missing-template-keyword}.
+
@item -Wno-multichar
@opindex Wno-multichar
@opindex Wmultichar
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 21fb5f7ebbc..dcda1cccedd 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -880,6 +880,10 @@ Wmissing-requires
C++ ObjC++ Var(warn_missing_requires) Init(1) Warning
Warn about likely missing requires keyword.
+Wmissing-template-keyword
+C++ ObjC++ Var(warn_missing_template_keyword) Init(1) Warning
+Warn when the template keyword is missing after a member access token in a dependent member access expression if that member is a template.
+
Wmultistatement-macros
C ObjC C++ ObjC++ Var(warn_multistatement_macros) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
Warn about unsafe macros expanding to multiple statements used as a body of a clause such as if, else, while, switch, or for.
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index f40e707d47c..c06ed5e4dbd 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -1275,17 +1275,32 @@ cp_lexer_rollback_tokens (cp_lexer* lexer)
lexer->next_token = lexer->saved_tokens.pop ();
}
-/* RAII wrapper around the above functions, with sanity checking. Creating
- a variable saves tokens, which are committed when the variable is
- destroyed unless they are explicitly rolled back by calling the rollback
- member function. */
+/* Determines what saved_token_sentinel does when going out of scope. */
+
+enum saved_token_sentinel_mode {
+ STS_COMMIT,
+ STS_ROLLBACK,
+ STS_DONOTHING
+};
+
+/* RAII wrapper around the above functions, with sanity checking (the token
+ stream should be the same at the point of instantiation as it is at the
+ point of destruction).
+
+ Creating a variable saves tokens. MODE determines what happens when the
+ object is destroyed. STS_COMMIT commits tokens (default),
+ STS_ROLLBACK rolls-back and STS_DONOTHING does nothing. Calling
+ rollback() will immediately roll-back tokens and set MODE to
+ STS_DONOTHING. */
struct saved_token_sentinel
{
cp_lexer *lexer;
unsigned len;
- bool commit;
- saved_token_sentinel(cp_lexer *lexer): lexer(lexer), commit(true)
+ saved_token_sentinel_mode mode;
+ saved_token_sentinel (cp_lexer *_lexer,
+ saved_token_sentinel_mode _mode = STS_COMMIT)
+ : lexer (_lexer), mode (_mode)
{
len = lexer->saved_tokens.length ();
cp_lexer_save_tokens (lexer);
@@ -1293,12 +1308,15 @@ struct saved_token_sentinel
void rollback ()
{
cp_lexer_rollback_tokens (lexer);
- commit = false;
+ mode = STS_DONOTHING;
}
- ~saved_token_sentinel()
+ ~saved_token_sentinel ()
{
- if (commit)
+ if (mode == STS_COMMIT)
cp_lexer_commit_tokens (lexer);
+ else if (mode == STS_ROLLBACK)
+ cp_lexer_rollback_tokens (lexer);
+
gcc_assert (lexer->saved_tokens.length () == len);
}
};
@@ -2771,7 +2789,11 @@ static void cp_parser_skip_to_end_of_block_or_statement
(cp_parser *);
static bool cp_parser_skip_to_closing_brace
(cp_parser *);
-static void cp_parser_skip_to_end_of_template_parameter_list
+static bool cp_parser_skip_entire_template_parameter_list
+ (cp_parser *);
+static void cp_parser_require_end_of_template_parameter_list
+ (cp_parser *);
+static bool cp_parser_skip_to_end_of_template_parameter_list
(cp_parser *);
static void cp_parser_skip_to_pragma_eol
(cp_parser*, cp_token *);
@@ -6129,47 +6151,41 @@ cp_parser_id_expression (cp_parser *parser,
template_keyword_p)
!= NULL_TREE);
+ cp_expr id = NULL_TREE;
+ tree scope = parser->scope;
+
+ /* Peek at the next token. */
+ cp_token *token = cp_lexer_peek_token (parser->lexer);
+
/* If there is a nested-name-specifier, then we are looking at
the first qualified-id production. */
if (nested_name_specifier_p)
{
- tree saved_scope;
tree saved_object_scope;
tree saved_qualifying_scope;
- cp_expr unqualified_id;
- bool is_template;
/* See if the next token is the `template' keyword. */
if (!template_p)
- template_p = &is_template;
+ template_p = &template_keyword_p;
*template_p = cp_parser_optional_template_keyword (parser);
/* Name lookup we do during the processing of the
unqualified-id might obliterate SCOPE. */
- saved_scope = parser->scope;
saved_object_scope = parser->object_scope;
saved_qualifying_scope = parser->qualifying_scope;
/* Process the final unqualified-id. */
- unqualified_id = cp_parser_unqualified_id (parser, *template_p,
- check_dependency_p,
- declarator_p,
- /*optional_p=*/false);
+ id = cp_parser_unqualified_id (parser, *template_p,
+ check_dependency_p,
+ declarator_p,
+ /*optional_p=*/false);
/* Restore the SAVED_SCOPE for our caller. */
- parser->scope = saved_scope;
+ parser->scope = scope;
parser->object_scope = saved_object_scope;
parser->qualifying_scope = saved_qualifying_scope;
-
- return unqualified_id;
}
/* Otherwise, if we are in global scope, then we are looking at one
of the other qualified-id productions. */
else if (global_scope_p)
{
- cp_token *token;
- tree id;
-
- /* Peek at the next token. */
- token = cp_lexer_peek_token (parser->lexer);
-
/* If it's an identifier, and the next token is not a "<", then
we can avoid the template-id case. This is an optimization
for this common case. */
@@ -6195,11 +6211,15 @@ cp_parser_id_expression (cp_parser *parser,
switch (token->type)
{
case CPP_NAME:
- return cp_parser_identifier (parser);
+ id = cp_parser_identifier (parser);
+ break;
case CPP_KEYWORD:
if (token->keyword == RID_OPERATOR)
- return cp_parser_operator_function_id (parser);
+ {
+ id = cp_parser_operator_function_id (parser);
+ break;
+ }
/* Fall through. */
default:
@@ -6208,10 +6228,44 @@ cp_parser_id_expression (cp_parser *parser,
}
}
else
- return cp_parser_unqualified_id (parser, template_keyword_p,
+ {
+ if (!scope)
+ scope = parser->context->object_type;
+ id = cp_parser_unqualified_id (parser, template_keyword_p,
/*check_dependency_p=*/true,
declarator_p,
optional_p);
+ }
+
+ if (id && TREE_CODE (id) == IDENTIFIER_NODE
+ && warn_missing_template_keyword
+ && !template_keyword_p
+ /* Don't warn if we're looking inside templates. */
+ && check_dependency_p
+ /* In a template argument list a > could be closing
+ the enclosing targs. */
+ && !parser->in_template_argument_list_p
+ && scope && dependentish_scope_p (scope)
+ /* Don't confuse an ill-formed constructor declarator for a missing
+ template keyword in a return type. */
+ && !(declarator_p && constructor_name_p (id, scope))
+ && cp_parser_nth_token_starts_template_argument_list_p (parser, 1)
+ && 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_peek_token (parser->lexer)->type
+ <= CPP_LAST_PUNCTUATOR))
+ warning_at (token->location, OPT_Wmissing_template_keyword,
+ "expected %qs keyword before dependent "
+ "template name", "template");
+ }
+
+ return id;
}
/* Parse an unqualified-id.
@@ -11472,7 +11526,7 @@ cp_parser_lambda_declarator_opt (cp_parser* parser, tree lambda_expr)
cp_lexer_consume_token (parser->lexer);
template_param_list = cp_parser_template_parameter_list (parser);
- cp_parser_skip_to_end_of_template_parameter_list (parser);
+ cp_parser_require_end_of_template_parameter_list (parser);
/* We may have a constrained generic lambda; parse the requires-clause
immediately after the template-parameter-list and combine with any
@@ -12353,11 +12407,11 @@ cp_parser_statement (cp_parser* parser, tree in_statement_expr,
{
if (saved_tokens.lexer == lexer)
{
- if (saved_tokens.commit)
+ if (saved_tokens.mode == STS_COMMIT)
cp_lexer_commit_tokens (lexer);
gcc_assert (lexer->saved_tokens.length () == saved_tokens.len);
saved_tokens.lexer = parser->lexer;
- saved_tokens.commit = false;
+ saved_tokens.mode = STS_DONOTHING;
saved_tokens.len = parser->lexer->saved_tokens.length ();
}
cp_lexer_destroy (lexer);
@@ -31493,7 +31547,7 @@ cp_parser_explicit_template_declaration (cp_parser* parser, bool member_p)
}
/* Look for the `>'. */
- cp_parser_skip_to_end_of_template_parameter_list (parser);
+ cp_parser_require_end_of_template_parameter_list (parser);
/* Manage template requirements */
if (flag_concepts)
@@ -32017,7 +32071,7 @@ cp_parser_enclosed_template_argument_list (cp_parser* parser)
}
}
else
- cp_parser_skip_to_end_of_template_parameter_list (parser);
+ cp_parser_require_end_of_template_parameter_list (parser);
/* The `>' token might be a greater-than operator again now. */
parser->greater_than_is_operator_p
= saved_greater_than_is_operator_p;
@@ -32912,11 +32966,45 @@ cp_parser_require (cp_parser* parser,
}
}
-/* An error message is produced if the next token is not '>'.
- All further tokens are skipped until the desired token is
- found or '{', '}', ';' or an unbalanced ')' or ']'. */
+/* Skip an entire parameter list from start to finish. The next token must
+ be the initial "<" of the parameter list. Returns true on success and
+ false otherwise. */
+
+static bool
+cp_parser_skip_entire_template_parameter_list (cp_parser* parser)
+{
+ /* Consume the "<" because cp_parser_skip_to_end_of_template_parameter_list
+ requires it. */
+ cp_lexer_consume_token (parser->lexer);
+ return cp_parser_skip_to_end_of_template_parameter_list (parser);
+}
+
+/* Ensure we are at the end of a template parameter list. If we are, return.
+ If we are not, something has gone wrong, in which case issue an error and
+ skip to end of the parameter list. */
static void
+cp_parser_require_end_of_template_parameter_list (cp_parser* parser)
+{
+ /* Are we ready, yet? If not, issue error message. */
+ if (cp_parser_require (parser, CPP_GREATER, RT_GREATER))
+ return;
+
+ cp_parser_skip_to_end_of_template_parameter_list (parser);
+}
+
+/* You should only call this function from inside a template parameter list
+ (i.e. the current token should at least be the initial "<" of the
+ parameter list). If you are skipping the entire list, it may be better to
+ use cp_parser_skip_entire_template_parameter_list.
+
+ Tokens are skipped until the final ">" is found, or if we see
+ '{', '}', ';', or if we find an unbalanced ')' or ']'.
+
+ Returns true if we successfully reached the end, and false if
+ something unexpected happened (e.g. end of file). */
+
+static bool
cp_parser_skip_to_end_of_template_parameter_list (cp_parser* parser)
{
/* Current level of '< ... >'. */
@@ -32924,10 +33012,6 @@ cp_parser_skip_to_end_of_template_parameter_list (cp_parser* parser)
/* Ignore '<' and '>' nested inside '( ... )' or '[ ... ]'. */
unsigned nesting_depth = 0;
- /* Are we ready, yet? If not, issue error message. */
- if (cp_parser_require (parser, CPP_GREATER, RT_GREATER))
- return;
-
/* Skip tokens until the desired token is found. */
while (true)
{
@@ -32951,7 +33035,7 @@ cp_parser_skip_to_end_of_template_parameter_list (cp_parser* parser)
spurious. Just consume the `>>' and stop; we've
already produced at least one error. */
cp_lexer_consume_token (parser->lexer);
- return;
+ return false;
}
/* Fall through for C++0x, so we handle the second `>' in
the `>>'. */
@@ -32962,7 +33046,7 @@ cp_parser_skip_to_end_of_template_parameter_list (cp_parser* parser)
{
/* We've reached the token we want, consume it and stop. */
cp_lexer_consume_token (parser->lexer);
- return;
+ return true;
}
break;
@@ -32974,7 +33058,7 @@ cp_parser_skip_to_end_of_template_parameter_list (cp_parser* parser)
case CPP_CLOSE_PAREN:
case CPP_CLOSE_SQUARE:
if (nesting_depth-- == 0)
- return;
+ return false;
break;
case CPP_EOF:
@@ -32983,7 +33067,7 @@ cp_parser_skip_to_end_of_template_parameter_list (cp_parser* parser)
case CPP_OPEN_BRACE:
case CPP_CLOSE_BRACE:
/* The '>' was probably forgotten, don't look further. */
- return;
+ return false;
default:
break;
diff --git a/gcc/testsuite/g++.dg/cpp0x/variadic-mem_fn2.C b/gcc/testsuite/g++.dg/cpp0x/variadic-mem_fn2.C
index 4a02ab22990..c343f32086d 100644
--- a/gcc/testsuite/g++.dg/cpp0x/variadic-mem_fn2.C
+++ b/gcc/testsuite/g++.dg/cpp0x/variadic-mem_fn2.C
@@ -5,5 +5,6 @@ template <class A0, class... As> struct tuple
tuple<As...> tail;
template <int Offset, class... More> int apply(const More&... more) {
return tail.apply<1>(more...); // { dg-error "" } needs .template
+ // { dg-warning "keyword before dependent template name" "" { target *-*-* } .-1 }
}
};
diff --git a/gcc/testsuite/g++.dg/template/dependent-name17.C b/gcc/testsuite/g++.dg/template/dependent-name17.C
new file mode 100644
index 00000000000..ff623e2d2ba
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/dependent-name17.C
@@ -0,0 +1,49 @@
+// C++ PR 70317
+// { dg-do compile }
+// { dg-prune-output "expected primary-expression" }
+
+template<class T> class mytemplateclass
+{
+public:
+ template<class U> void class_func() {}
+ template<class U> static void class_func_static() {}
+};
+
+class myclass
+{
+public:
+ int testint;
+ template<class U> void class_func() {}
+ template<class U> static void class_func_static() {}
+};
+
+template<class Y> void tests_func(mytemplateclass<Y> c, myclass c2)
+{
+ /* Dependent template accessors (ill-formed code). */
+ c.class_func<Y>(); // { dg-warning "keyword before dependent template name" }
+ (&c)->class_func<Y>(); // { dg-warning "keyword before dependent template name" }
+ mytemplateclass<Y>::class_func_static<Y>(); // { dg-warning "keyword before dependent template name" }
+
+ /* Dependent template accessors (well-formed code). */
+ c.template class_func<Y>();
+ (&c)->template class_func<Y>();
+ mytemplateclass<Y>::template class_func_static<Y>();
+
+ /* Non-dependent template accessors (well-formed code). */
+ c2.class_func<myclass>();
+ (&c2)->class_func<myclass>();
+ myclass::class_func_static<myclass>();
+}
+
+int main()
+{
+ mytemplateclass<myclass> c;
+ myclass c2;
+ tests_func<myclass>(c, c2);
+
+ c2.testint = 53;
+ /* Make sure this isn't treated as a template. */
+ bool testbool = c2.testint < 999 > 7;
+ /* This probably will be treated as a template initially but it should still work. */
+ testbool = c2.testint < 123 > (50);
+}
diff --git a/gcc/testsuite/g++.dg/template/dependent-name18.C b/gcc/testsuite/g++.dg/template/dependent-name18.C
new file mode 100644
index 00000000000..f6f0a27a0bd
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/dependent-name18.C
@@ -0,0 +1,5 @@
+template <bool B> struct A { };
+template <class T> void f()
+{
+ A<T::I < T::J>();
+}
--
2.27.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] c++: error message for dependent template members [PR70417]
2022-01-13 21:01 ` Jason Merrill
@ 2022-01-15 8:27 ` Anthony Sharp
2022-01-17 22:26 ` Jason Merrill
0 siblings, 1 reply; 12+ messages in thread
From: Anthony Sharp @ 2022-01-15 8:27 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches
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 <jason@redhat.com> 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 <class T> struct A
> > {
> > template <class U> void f(U u);
> > };
> >
> > template <class T> void g(A<T> 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 <bool B> struct A { };
> > template <class T> void f()
> > {
> > A<T::I < T::J>();
> > };
> >
> > 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!
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] c++: error message for dependent template members [PR70417]
2022-01-15 8:27 ` Anthony Sharp
@ 2022-01-17 22:26 ` Jason Merrill
0 siblings, 0 replies; 12+ messages in thread
From: Jason Merrill @ 2022-01-17 22:26 UTC (permalink / raw)
To: Anthony Sharp; +Cc: gcc-patches List
On Sat, Jan 15, 2022 at 3:28 AM Anthony Sharp <anthonysharp15@gmail.com>
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 <jason@redhat.com> 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 <class T> struct A
>> > {
>> > template <class U> void f(U u);
>> > };
>> >
>> > template <class T> void g(A<T> 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 <bool B> struct A { };
>> > template <class T> void f()
>> > {
>> > A<T::I < T::J>();
>> > };
>> >
>> > 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!
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-01-17 22:27 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-20 16:56 [PATCH] c++: error message for dependent template members [PR70417] Anthony Sharp
2021-08-27 21:33 ` Jason Merrill
[not found] ` <CA+Lh_AnijQzyM6qgwHS7hZqmA8uO8VahG5fmdqFMF6wRcrbefQ@mail.gmail.com>
2021-09-17 22:17 ` Anthony Sharp
2021-09-17 22:22 ` Anthony Sharp
2021-09-22 20:04 ` Jason Merrill
2021-10-10 11:28 ` Anthony Sharp
2021-10-19 19:15 ` Jason Merrill
2021-12-04 17:23 ` Anthony Sharp
2021-12-09 15:51 ` Jason Merrill
2022-01-13 21:01 ` Jason Merrill
2022-01-15 8:27 ` Anthony Sharp
2022-01-17 22:26 ` Jason Merrill
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).