[cfe-commits] r153488 - in /cfe/trunk: lib/Parse/ParseDecl.cpp test/Parser/cxx-class.cpp test/SemaCXX/copy-assignment.cpp

[Richard Smith]

On Mon, Mar 26, 2012 at 6:18 PM, Eli Friedman <eli.friedman at gmail.com>wrote:

> On Mon, Mar 26, 2012 at 5:56 PM, Richard Smith
> <richard-llvm at metafoo.co.uk> wrote:
> > Author: rsmith
> > Date: Mon Mar 26 19:56:56 2012
> > New Revision: 153488
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=153488&view=rev
> > Log:
> > When we see 'Class(X' or 'Class::Class(X' and we suspect that it names a
> > constructor, but X is not a known typename, check whether the tokens
> could
> > possibly match the syntax of a declarator before concluding that it isn't
> > a constructor. If it's definitely ill-formed, assume it is a constructor.
> >
> > Empirical evidence suggests that this pattern is much more often a
> > constructor with a typoed (or not-yet-declared) type name than any of the
> > other possibilities, so the extra cost of the check is not expected to be
> > problematic.
> >
> > Modified:
> >    cfe/trunk/lib/Parse/ParseDecl.cpp
> >    cfe/trunk/test/Parser/cxx-class.cpp
> >    cfe/trunk/test/SemaCXX/copy-assignment.cpp
> >
> > Modified: cfe/trunk/lib/Parse/ParseDecl.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseDecl.cpp?rev=153488&r1=153487&r2=153488&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/lib/Parse/ParseDecl.cpp (original)
> > +++ cfe/trunk/lib/Parse/ParseDecl.cpp Mon Mar 26 19:56:56 2012
> > @@ -3379,7 +3379,42 @@
> >   // Check whether the next token(s) are part of a declaration
> >   // specifier, in which case we have the start of a parameter and,
> >   // therefore, we know that this is a constructor.
> > -  bool IsConstructor = isDeclarationSpecifier();
> > +  bool IsConstructor = false;
> > +  if (isDeclarationSpecifier())
> > +    IsConstructor = true;
> > +  else if (Tok.is(tok::identifier) ||
> > +           (Tok.is(tok::annot_cxxscope) &&
> NextToken().is(tok::identifier))) {
> > +    // We've seen "C ( X" or "C ( X::Y", but "X" / "X::Y" is not a type.
> > +    // This might be a parenthesized member name, but is more likely to
> > +    // be a constructor declaration with an invalid argument type. Keep
> > +    // looking.
> > +    if (Tok.is(tok::annot_cxxscope))
> > +      ConsumeToken();
> > +    ConsumeToken();
> > +
> > +    // If this is not a constructor, we must be parsing a declarator,
> > +    // which must have one of the following syntactic forms:
> > +    switch (Tok.getKind()) {
> > +    case tok::l_paren:
> > +      // C(X   (   int));
> > +    case tok::l_square:
> > +      // C(X   [   5]);
> > +      // C(X   [   [attribute]]);
> > +    case tok::coloncolon:
> > +      // C(X   ::   Y);
> > +      // C(X   ::   *p);
> > +    case tok::r_paren:
> > +      // C(X   )
> > +      // Assume this isn't a constructor, rather than assuming it's a
> > +      // constructor with an unnamed parameter of an ill-formed type.
> > +      break;
>
> I really don't like adding new lists of tokens like this... can you at
> least leave a note pointing here from the place which actually handles
> this grammar?
>

Done, r153490. I'm not a huge fan of this approach either, but
isConstructorDeclarator is already in the business of encoding facts about
these parts of the grammar to disambiguate.

Looking more into the existing code, it appears we've already slipped up
here when a previous change added grammar to ParseDeclarator but didn't
update isConstructorDeclarator. We incorrectly disambiguate this valid
code, because we think the ellipsis unambiguously makes this a constructor:

template<typename...T> struct S { void f(S(...args[sizeof(T)])); };
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120326/e91d9f7e/attachment.html>