r217302 - Add error, recovery and fixit for "~A::A() {...}".

[Richard Smith]

On Sun, Jan 4, 2015 at 2:26 PM, Nico Weber <thakis at chromium.org> wrote:

> Sorry for the somewhat delayed review. Two points below.
>
> On Fri, Sep 5, 2014 at 7:06 PM, Richard Smith <richard-llvm at metafoo.co.uk>
> wrote:
>
>> Author: rsmith
>> Date: Fri Sep  5 21:06:12 2014
>> New Revision: 217302
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=217302&view=rev
>> Log:
>> Add error, recovery and fixit for "~A::A() {...}".
>>
>> Modified:
>>     cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
>>     cfe/trunk/lib/Parse/ParseExprCXX.cpp
>>     cfe/trunk/test/FixIt/fixit.cpp
>>     cfe/trunk/test/Parser/cxx-class.cpp
>>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td?rev=217302&r1=217301&r2=217302&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td Fri Sep  5
>> 21:06:12 2014
>> @@ -492,6 +492,8 @@ def err_constructor_bad_name : Error<
>>    "missing return type for function %0; did you mean the constructor
>> name %1?">;
>>  def err_destructor_tilde_identifier : Error<
>>    "expected a class name after '~' to name a destructor">;
>> +def err_destructor_tilde_scope : Error<
>> +  "'~' in destructor name should be after nested name specifier">;
>>  def err_destructor_template_id : Error<
>>    "destructor name %0 does not refer to a template">;
>>  def err_default_arg_unparsed : Error<
>>
>> Modified: cfe/trunk/lib/Parse/ParseExprCXX.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseExprCXX.cpp?rev=217302&r1=217301&r2=217302&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/Parse/ParseExprCXX.cpp (original)
>> +++ cfe/trunk/lib/Parse/ParseExprCXX.cpp Fri Sep  5 21:06:12 2014
>> @@ -2452,10 +2452,29 @@ bool Parser::ParseUnqualifiedId(CXXScope
>>        return true;
>>      }
>>
>> +    // If the user wrote ~T::T, correct it to T::~T.
>> +    if (!TemplateSpecified && NextToken().is(tok::coloncolon)) {
>> +      if (SS.isSet()) {
>> +        AnnotateScopeToken(SS, /*NewAnnotation*/true);
>> +        SS.clear();
>> +      }
>> +      if (ParseOptionalCXXScopeSpecifier(SS, ObjectType,
>> EnteringContext))
>> +        return true;
>>
>
> This scope spec never reaches a DeclaratorScopeObj. Normally,
> ParseDirectDeclarator() calls DeclScopeObj.EnterDeclaratorScope()
> if Actions.ShouldEnterDeclaratorScope(), but this isn't done after calling
> ParseUnqualifiedId(). As a consequence, this parses:
>
>     struct B {
>       class F {};
>       ~B() throw(F);
>     };
>     B::~B() throw(F) {}
>
> While this doesn't:
>
>     struct B {
>       class F {};
>       ~B() throw(F);
>     };
>     ~B::B() throw(F) {} // Complains about not knowing F, suggests B::F
>
>
>> +      if (Tok.isNot(tok::identifier) || NextToken().is(tok::coloncolon))
>> {
>> +        Diag(TildeLoc, diag::err_destructor_tilde_scope);
>> +        return true;
>> +      }
>> +
>> +      // Recover as if the tilde had been written before the identifier.
>>
>
> This angers and confuses sema if getDestructorName() below is called for
> an incomplete type:
>
>     struct B;
>     ~B::B() {}
>
> => Assertion failed: ((!isa<TagDecl>(LookupCtx) ||
> LookupCtx->isDependentContext() ||
> cast<TagDecl>(LookupCtx)->isCompleteDefinition() ||
> cast<TagDecl>(LookupCtx)->isBeingDefined()) && "Declaration context must
> already be complete!"), function LookupQualifiedName, file
> /Users/thakis/src/llvm-rw/tools/clang/lib/Sema/SemaLookup.cpp, line 1595.
> 6  clang::Sema::LookupQualifiedName()
> 7  clang::Sema::getDestructorName()
> 8  clang::Parser::ParseUnqualifiedId()
> 9  clang::Parser::ParseDirectDeclarator()
>
> Calling ActOnCXXEnterDeclaratorScope() (via DeclaratorScopeObj) would fix
> this as it checks for complete types in the declarator scope, but it's not
> obvious to me how to do this from here – and doing it in
> ParseDirectDeclarator() after this function has been called would mean it'd
> happen after this function calls getDestructorName().
>

Yuck, thanks. Fixed in r226067. For now I made us enter the scope twice
(once within ParseUnqualifiedId and again once we're done parsing it), but
ideas for better options would be welcome.


> +      Diag(TildeLoc, diag::err_destructor_tilde_scope)
>> +        << FixItHint::CreateRemoval(TildeLoc)
>> +        << FixItHint::CreateInsertion(Tok.getLocation(), "~");
>> +    }
>> +
>>      // Parse the class-name (or template-name in a simple-template-id).
>>      IdentifierInfo *ClassName = Tok.getIdentifierInfo();
>>      SourceLocation ClassNameLoc = ConsumeToken();
>> -
>> +
>>      if (TemplateSpecified || Tok.is(tok::less)) {
>>        Result.setDestructorName(TildeLoc, ParsedType(), ClassNameLoc);
>>        return ParseUnqualifiedIdTemplateId(SS, TemplateKWLoc,
>> @@ -2463,7 +2482,7 @@ bool Parser::ParseUnqualifiedId(CXXScope
>>                                            EnteringContext, ObjectType,
>>                                            Result, TemplateSpecified);
>>      }
>> -
>> +
>>      // Note that this is a destructor name.
>>      ParsedType Ty = Actions.getDestructorName(TildeLoc, *ClassName,
>>                                                ClassNameLoc,
>> getCurScope(),
>>
>> Modified: cfe/trunk/test/FixIt/fixit.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/FixIt/fixit.cpp?rev=217302&r1=217301&r2=217302&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/test/FixIt/fixit.cpp (original)
>> +++ cfe/trunk/test/FixIt/fixit.cpp Fri Sep  5 21:06:12 2014
>> @@ -308,6 +308,13 @@ namespace dtor_fixit {
>>      ~bar() { }  // expected-error {{expected the class name after '~' to
>> name a destructor}}
>>      // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:6-[[@LINE-1]]:9}:"foo"
>>    };
>> +
>> +  class bar {
>> +    ~bar();
>> +  };
>> +  ~bar::bar() {} // expected-error {{'~' in destructor name should be
>> after nested name specifier}}
>> +  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:4}:""
>> +  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:9-[[@LINE-2]]:9}:"~"
>>  }
>>
>>  namespace PR5066 {
>>
>> Modified: cfe/trunk/test/Parser/cxx-class.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/cxx-class.cpp?rev=217302&r1=217301&r2=217302&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/test/Parser/cxx-class.cpp (original)
>> +++ cfe/trunk/test/Parser/cxx-class.cpp Fri Sep  5 21:06:12 2014
>> @@ -139,6 +139,20 @@ namespace CtorErrors {
>>    };
>>  }
>>
>> +namespace DtorErrors {
>> +  struct A { ~A(); } a;
>> +  ~A::A() {} // expected-error {{'~' in destructor name should be after
>> nested name specifier}} expected-note {{previous}}
>> +  A::~A() {} // expected-error {{redefinition}}
>> +
>> +  struct B { ~B(); } *b;
>> +  DtorErrors::~B::B() {} // expected-error {{'~' in destructor name
>> should be after nested name specifier}}
>> +
>> +  void f() {
>> +    a.~A::A(); // expected-error {{'~' in destructor name should be
>> after nested name specifier}}
>> +    b->~DtorErrors::~B::B(); // expected-error {{'~' in destructor name
>> should be after nested name specifier}}
>> +  }
>> +}
>> +
>>  namespace BadFriend {
>>    struct A {
>>      friend int : 3; // expected-error {{friends can only be classes or
>> functions}}
>>
>>
>> _______________________________________________
>> 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/20150114/079a5889/attachment.html>