[cfe-commits] r157085 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaTemplate.cpp test/SemaTemplate/typename-specifier.cpp
Kaelyn Uhrain
rikka at google.com
Thu Jun 7 18:09:02 PDT 2012
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).
> + } else if (DependentScopeDeclRefExpr *ArgExpr =
> > + dyn_cast<DependentScopeDeclRefExpr>(Arg.getAsExpr())) {
> > + SS.Adopt(ArgExpr->getQualifierLoc());
> > + NameInfo = ArgExpr->getNameInfo();
> > + } else if (CXXDependentScopeMemberExpr *ArgExpr =
> > + dyn_cast<CXXDependentScopeMemberExpr>(Arg.getAsExpr())) {
> > + SS.Adopt(ArgExpr->getQualifierLoc());
> > + NameInfo = ArgExpr->getMemberNameInfo();
>
> This should check that the base of the dependent-scoped member access
> expression is actually the implicit 'this'. We currently end up inserting
> 'typename' where we shouldn't:
>
> test/SemaTemplate/typename-specifier.cpp:139:10: error: template argument
> for
> template type parameter must be a type; did you forget 'typename'?
> pair<this->ExampleItemSet::iterator, int> i; // expected-error...
> ^
>
Good point; fixed. The "typename" stuff delves into an area of C++
templates I've never used, and adding the suggestion turned out to be more
challenging than I'd expected--it certainly required a lot more knowledge
of template syntax and of how Clang deals with them than I would have
guessed from the relatively straight-forward and self-contained error and
recovery handling I'd encountered working on typo correction (emphasis on
"relatively").
> + }
> > +
> > + if (NameInfo.getName()) {
>
> This should be limited to identifiers, because we don't want to correct
> here:
>
> test/SemaTemplate/typename-specifier.cpp:139:10: error: template argument
> for
> template type parameter must be a type; did you forget 'typename'?
> pair<ExampleItemSet::operator[], int> i;
> ^
>
Also fixed.
> > + 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.
>
> > + 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.
Thanks!
Kaelyn
> - Doug
>
> > + }
> > + // fallthrough
> > + }
> > default: {
> > // We have a template type parameter but the template argument
> > // is not a type.
> >
> > Modified: cfe/trunk/test/SemaTemplate/typename-specifier.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaTemplate/typename-specifier.cpp?rev=157085&r1=157084&r2=157085&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/test/SemaTemplate/typename-specifier.cpp (original)
> > +++ cfe/trunk/test/SemaTemplate/typename-specifier.cpp Fri May 18
> 18:42:49 2012
> > @@ -115,3 +115,37 @@
> > using typename BasicGeometry<mydim, int>::operator[]; //
> expected-error {{typename is allowed for identifiers only}}
> > };
> > }
> > +
> > +
> > +namespace missing_typename {
> > +template <class T1, class T2> struct pair {}; // expected-note 5
> {{template parameter is declared here}}
> > +
> > +template <class T1, class T2>
> > +struct map {
> > + typedef T1* iterator;
> > +};
> > +
> > +template <class T>
> > +class ExampleClass1 {
> > + struct ExampleItem;
> > +
> > +
> > + struct ExampleItemSet {
> > + typedef ExampleItem* iterator;
> > + };
> > +
> > + void foo() {
> > + pair<ExampleItemSet::iterator, int> i; // expected-error {{template
> argument for template type parameter must be a type; did you forget
> 'typename'?}}
> > + }
> > + pair<ExampleItemSet::iterator, int> elt; // expected-error {{template
> argument for template type parameter must be a type; did you forget
> 'typename'?}}
> > +
> > +
> > + typedef map<int, ExampleItem*> ExampleItemMap;
> > +
> > + static void bar() {
> > + pair<ExampleItemMap::iterator, int> i; // expected-error {{template
> argument for template type parameter must be a type; did you forget
> 'typename'?}}
> > + }
> > + pair<ExampleItemMap::iterator, int> entry; // expected-error
> {{template argument for template type parameter must be a type; did you
> forget 'typename'?}}
> > + pair<bar, int> foobar; // expected-error {{template argument for
> template type parameter must be a type}}
> > +};
> > +} // namespace missing_typename
> >
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120607/4d1b5b16/attachment.html>
More information about the cfe-commits
mailing list