[cfe-commits] r157085 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaTemplate.cpp test/SemaTemplate/typename-specifier.cpp

[Douglas Gregor]

On Jun 7, 2012, at 6:09 PM, Kaelyn Uhrain wrote:

> On Thu, Jun 7, 2012 at 3:47 PM, Douglas Gregor <dgregor at apple.com> wrote:
> 
> On May 18, 2012, at 4:42 PM, Kaelyn Uhrain wrote:
> 
> > Author: rikka
> > Date: Fri May 18 18:42:49 2012
> > New Revision: 157085
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=157085&view=rev
> > Log:
> > Suggest adding 'typename' when it would make the compiler
> > accept the template argument expression as a type.
> >
> > Modified:
> >    cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> >    cfe/trunk/lib/Sema/SemaTemplate.cpp
> >    cfe/trunk/test/SemaTemplate/typename-specifier.cpp
> >
> > Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=157085&r1=157084&r2=157085&view=diff
> > ==============================================================================
> > --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> > +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Fri May 18 18:42:49 2012
> > @@ -2361,6 +2361,8 @@
> > def note_member_of_template_here : Note<"member is declared here">;
> > def err_template_arg_must_be_type : Error<
> >   "template argument for template type parameter must be a type">;
> > +def err_template_arg_must_be_type_suggest : Error<
> > +  "template argument for template type parameter must be a type; did you forget 'typename'?">;
> > def err_template_arg_must_be_expr : Error<
> >   "template argument for non-type template parameter must be an expression">;
> > def err_template_arg_nontype_ambig : Error<
> >
> > Modified: cfe/trunk/lib/Sema/SemaTemplate.cpp
> > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaTemplate.cpp?rev=157085&r1=157084&r2=157085&view=diff
> > ==============================================================================
> > --- cfe/trunk/lib/Sema/SemaTemplate.cpp (original)
> > +++ cfe/trunk/lib/Sema/SemaTemplate.cpp Fri May 18 18:42:49 2012
> > @@ -2438,6 +2438,45 @@
> >
> >     return true;
> >   }
> > +  case TemplateArgument::Expression: {
> > +    // We have a template type parameter but the template argument is an
> > +    // expression; see if maybe it is missing the "typename" keyword.
> > +    CXXScopeSpec SS;
> > +    DeclarationNameInfo NameInfo;
> > +
> > +    if (DeclRefExpr *ArgExpr = dyn_cast<DeclRefExpr>(Arg.getAsExpr())) {
> > +      SS.Adopt(ArgExpr->getQualifierLoc());
> > +      NameInfo = ArgExpr->getNameInfo();
> 
> Shouldn't we check that there is a nested-name-specifier here?
> 
> As far as I'm aware, there is no need. The code below is not dependent on the CXXScopeSpec being valid, and if there isn't a nested-name-specifier then the CXXScopeSpec will be invalid after adopting an empty NestedNameSpecifierLoc. Plus, from what I can tell, any checks for whether the expression needs to have a nested-name-specifier are handled elsewhere (IIRC they're triggered before this code is reached).

Okay, this seems to work because we can't get NotFoundInCurrentInstantiation with unqualified lookup.

> > +      LookupResult Result(*this, NameInfo, LookupOrdinaryName);
> > +      LookupParsedName(Result, CurScope, &SS);
> > +
> > +      bool CouldBeType = Result.getResultKind() ==
> > +          LookupResult::NotFoundInCurrentInstantiation;
> > +
> > +      for (LookupResult::iterator I = Result.begin(), IEnd = Result.end();
> > +           !CouldBeType && I != IEnd; ++I) {
> > +        CouldBeType = isa<TypeDecl>(*I);
> > +      }
> 
> I don't understand why we have this loop here. If the name lookup is able to find a type, we wouldn't be here in this code. It seems like the only interesting case is the NotFoundInCurrentInstantiation case.
> 
> D'oh. It was left over from when I was trying to figure out how to look up whether an identifier might be a type and even how to get the nested-name-specifier in a form that I could use for the lookups. IIRC at least one of the avenues I'd tried needed the loop to handle lookups that returned multiple decls.

Come to think of it, do you even need to do a lookup here? A DeclRefExpr won't refer to a type, so it's actually not an interesting case. DependentScopeDeclRefExpr and CXXDependentScopeMemberExpr (the latter only when it has a qualifier) are the interesting cases, and we wouldn't have built those nodes unless name lookup for the entity ended up with NotFoundInCurrentInstantiation. 

> 
> > +      if (CouldBeType) {
> > +        SourceLocation Loc = AL.getSourceRange().getBegin();
> > +        Diag(Loc, diag::err_template_arg_must_be_type_suggest);
> > +        Diag(Param->getLocation(), diag::note_template_param_here);
> > +        return true;
> > +      }
> 
> Did you mean for this to be a Fix-It? If so, it needs to recover by changing the template argument itself. If you're not planning on doing that work, it's fine, but please leave a FIXME to say that we'd like to do that work.
> 
> I've added a FIXME since a Fix-It was beyond what I had able to figure out while muddling through all of the template code...
> 
> The changes mentioned above along with a couple more tests have been committed as r158185.

Okay, thanks for the fixes!

	- Doug
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120607/61ff0d8b/attachment.html>