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
From ecccf8103b2368564d073d4b85efd9484448a59f Mon Sep 17 00:00:00 2001
From: Jason Merrill <ja...@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
