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 <anthonyshar...@gmail.com> wrote:
Re-adding gcc-patches@gcc.gnu.org.
---------- Forwarded message ---------
From: Anthony Sharp <anthonyshar...@gmail.com>
Date: Fri, 17 Sept 2021 at 23:11
Subject: Re: [PATCH] c++: error message for dependent template members [PR70417]
To: Jason Merrill <ja...@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
