rsmith added a comment.
Thank you, this is looking really good.
================
Comment at: include/clang/Parse/Parser.h:2054
+ case DeclSpecContext::DSC_template_param:
+ case DeclSpecContext::DSC_template_type_arg:
+ case DeclSpecContext::DSC_normal:
----------------
Rakete1111 wrote:
> rsmith wrote:
> > This doesn't seem right to me. Doesn't this mean that `X<T::U>` will change
> > from being a non-type template argument to being a type template argument?
> >
> > Maybe that's a bug in the wording paper, though, since that context *is* a
> > /type-id/.
> AFAIK no it won't, but I can't really tell if that's the right behavior or
> not. This patch doesn't touch the `X<T::U>` case.
I don't think I agree. `ParseTemplateArgument`, after disambiguation, parses a
type name in `TemplateArgContext`, which results in a `DeclSpecContext` of
`DSC_template_type_arg`, which means that we treat the context as an implicit
typename context.
It looks to me like that will cause us to accept implicit `typename` in cases
that we can disambiguate as being type template arguments, which would be
wrong. For example, it looks like `X<const T::U>` would be incorrectly
accepted: `typename` is required here because this type-id is not an implicit
typename context.
Perhaps the right way to fix this would be to change
`Parser::getDeclSpecContextFromDeclaratorContext` to map `TemplateArgContext`
to a different kind of `DeclSpecContext` than `TemplateTypeArgContext` so that
we can tell the difference here.
================
Comment at: lib/Parse/ParseDecl.cpp:6433
- ParseDeclarationSpecifiers(DS);
+ bool AllowImplicitTypename;
+ if (D.getContext() == DeclaratorContext::MemberContext ||
----------------
We should determine this up-front rather than computing it once per parameter
that we parse.
================
Comment at: lib/Parse/ParseDecl.cpp:6439
+ AllowImplicitTypename =
+ D.getCXXScopeSpec().isSet() && Actions.isDeclaratorFunctionLike(D);
----------------
I don't think you need to do name lookup here. I think you can use
`D.isFunctionDeclaratorAFunctionDeclaration()` instead of
`isDeclaratorFunctionLike(D)` -- because we've already committed to parsing
this as a function declaration in that case, a qualified function name will
either redeclare something from its context (in which case implicit typename is
permitted) or be ill-formed.
================
Comment at: lib/Parse/Parser.cpp:1518
+ if (TryAnnotateTypeOrScopeTokenAfterScopeSpec(
+ SS, !WasScopeAnnotation, /*AllowImplicitTypename=*/true))
return ANK_Error;
----------------
What justifies allowing implicit typename here? Don't we need the caller to
tell us if that's OK?
... actually, perhaps this tentatively-declared identifiers logic should only
be performed if `SS.isEmpty()` (because a tentatively-declared identifier can
never be found within a scope named by a nested-name-specifier), at which point
the value of this flag doesn't matter.
================
Comment at: lib/Parse/Parser.cpp:1783
+ AllowImplicitTypename &&
+ getCurScope()->isFunctionPrototypeScope())) {
SourceLocation BeginLoc = Tok.getLocation();
----------------
Do we need this scope check? (It would seem preferable to trust the caller to
have passed in the right flag value, if we can.)
================
Comment at: lib/Sema/Sema.cpp:2006-2019
+bool Sema::isDeclaratorFunctionLike(const Declarator &D) {
+ assert(D.getCXXScopeSpec().isSet() &&
+ "can only be called for qualified names");
+ LookupResult LR(*this, D.getIdentifier(), D.getBeginLoc(),
LookupOrdinaryName,
+ ForVisibleRedeclaration);
+ DeclContext *DC = computeDeclContext(D.getCXXScopeSpec());
+ if (!DC)
----------------
Some thoughts on this:
* Can this be unified with the lookup code in `HandleDeclarator`? This is
really the same lookup, repeated in two places.
* It'd be nice to cache this lookup, rather than performing it three times
(once when disambiguating a parenthesized initializer from a function
declaration, once when we're about to parse a parameter-declaration-clause, and
once in `HandleDeclarator` after parsing completes -- though at least that's
reduced to two lookups if you make the change I suggested in
`ParseParameterDeclarationClause`)
* If we don't do the caching, what happens if lookup fails due to ambiguity?
Do we get the same error multiple times (once for each time we perform the
lookup)?
================
Comment at: lib/Sema/Sema.cpp:2010
+ LookupResult LR(*this, D.getIdentifier(), D.getBeginLoc(),
LookupOrdinaryName,
+ ForVisibleRedeclaration);
+ DeclContext *DC = computeDeclContext(D.getCXXScopeSpec());
----------------
Should this be `forRedeclarationInCurContext()`?
================
Comment at: lib/Sema/Sema.cpp:2011
+ ForVisibleRedeclaration);
+ DeclContext *DC = computeDeclContext(D.getCXXScopeSpec());
+ if (!DC)
----------------
`EnteringContext` should presumably be `true` here, so that we can look into
class templates.
================
Comment at: lib/Sema/Sema.cpp:2016
+ LookupQualifiedName(LR, DC);
+ return std::all_of(LR.begin(), LR.end(), [](Decl *Dcl) {
+ return isa<FunctionDecl>(Dcl) || isa<FunctionTemplateDecl>(Dcl);
----------------
`UsingDecl`s in this set should not disqualify us from the "function-like"
interpretation.
================
Comment at: lib/Sema/Sema.cpp:2017
+ return std::all_of(LR.begin(), LR.end(), [](Decl *Dcl) {
+ return isa<FunctionDecl>(Dcl) || isa<FunctionTemplateDecl>(Dcl);
+ });
----------------
I think you need to `getUnderlyingDecl` of `Dcl` here; lookup might find
`UsingShadowDecl`s that you need to look through.
================
Comment at: test/SemaCXX/unknown-type-name.cpp:121-122
template<typename T>
-A<T>::g() { } // expected-error{{requires a type specifier}}
+A<T>::g() { } // expected-error{{expected unqualified-id}}
+// expected-warning@-1{{implicit 'typename' is a C++2a extension}}
----------------
Rakete1111 wrote:
> rsmith wrote:
> > rsmith wrote:
> > > This is a diagnostic quality regression. Perhaps that's an inevitable
> > > consequence of P0634, but we should at least try to do better.
> > This is marked "done" but doesn't seem to be done?
> Oops, don't know why I did that. I can't figure out a way to fix this. I can
> implement a basic heuristic to detect some very basic cases like this one,
> but I don't think it's worth it.
OK, fair enough. This one does seem hard to diagnose well.
Repository:
rC Clang
https://reviews.llvm.org/D53847
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
