r186199 - Unrevert r186040, reverted in r186185, with fix for PR16597.

Chandler Carruth chandlerc at google.com
Mon Jul 15 05:17:49 PDT 2013


I still think this is completely busted:

% cat x.cc
namespace N {
  struct X {
    friend void f();
  };
  extern void f();
  struct Y {
    friend void f();
  };
}

int main() {
  N::f();
}

% ./bin/clang -fsyntax-only x.cc
x.cc:12:6: error: no member named 'f' in namespace 'N'
  N::f();
  ~~~^
1 error generated.

Huh?

Commenting out the friend on line 7 fixes this. Old clang and GCC don't
behave this way, and I can't come up with a reason why the new behavior
would be correct, but maybe I'm missing something?


On Fri, Jul 12, 2013 at 1:38 PM, Richard Smith
<richard-llvm at metafoo.co.uk>wrote:

> Author: rsmith
> Date: Fri Jul 12 15:38:49 2013
> New Revision: 186199
>
> URL: http://llvm.org/viewvc/llvm-project?rev=186199&view=rev
> Log:
> Unrevert r186040, reverted in r186185, with fix for PR16597.
>
> Original commit log:
>   If we friend a declaration twice, that should not make it visible to
>   name lookup in the surrounding context. Slightly rework how we handle
>   friend declarations to inherit the visibility of the prior
>   declaration, rather than setting a friend declaration to be visible
>   whenever there was a prior declaration.
>
> Modified:
>     cfe/trunk/include/clang/AST/Decl.h
>     cfe/trunk/include/clang/AST/DeclBase.h
>     cfe/trunk/lib/Sema/SemaDecl.cpp
>     cfe/trunk/lib/Sema/SemaLookup.cpp
>     cfe/trunk/lib/Sema/SemaTemplate.cpp
>     cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
>     cfe/trunk/test/CXX/temp/temp.decls/temp.friend/p4.cpp
>     cfe/trunk/test/SemaCXX/friend.cpp
>
> Modified: cfe/trunk/include/clang/AST/Decl.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Decl.h?rev=186199&r1=186198&r2=186199&view=diff
>
> ==============================================================================
> --- cfe/trunk/include/clang/AST/Decl.h (original)
> +++ cfe/trunk/include/clang/AST/Decl.h Fri Jul 12 15:38:49 2013
> @@ -3402,6 +3402,12 @@ void Redeclarable<decl_type>::setPreviou
>    First->RedeclLink = LatestDeclLink(static_cast<decl_type*>(this));
>    assert(!isa<NamedDecl>(static_cast<decl_type*>(this)) ||
>
> cast<NamedDecl>(static_cast<decl_type*>(this))->isLinkageValid());
> +
> +  // If the declaration was previously visible, a redeclaration of it
> remains
> +  // visible even if it wouldn't be visible by itself.
> +  static_cast<decl_type*>(this)->IdentifierNamespace |=
> +    First->getIdentifierNamespace() &
> +    (Decl::IDNS_Ordinary | Decl::IDNS_Tag | Decl::IDNS_Type);
>  }
>
>  // Inline function definitions.
>
> Modified: cfe/trunk/include/clang/AST/DeclBase.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclBase.h?rev=186199&r1=186198&r2=186199&view=diff
>
> ==============================================================================
> --- cfe/trunk/include/clang/AST/DeclBase.h (original)
> +++ cfe/trunk/include/clang/AST/DeclBase.h Fri Jul 12 15:38:49 2013
> @@ -295,6 +295,8 @@ protected:
>    friend class ASTReader;
>    friend class LinkageComputer;
>
> +  template<typename decl_type> friend class Redeclarable;
> +
>  private:
>    void CheckAccessDeclContext() const;
>
> @@ -824,7 +826,7 @@ public:
>    /// class, but in the semantic context of the actual entity.  This
> property
>    /// applies only to a specific decl object;  other redeclarations of the
>    /// same entity may not (and probably don't) share this property.
> -  void setObjectOfFriendDecl(bool PreviouslyDeclared) {
> +  void setObjectOfFriendDecl(bool PerformFriendInjection = false) {
>      unsigned OldNS = IdentifierNamespace;
>      assert((OldNS & (IDNS_Tag | IDNS_Ordinary |
>                       IDNS_TagFriend | IDNS_OrdinaryFriend)) &&
> @@ -833,15 +835,20 @@ public:
>                         IDNS_TagFriend | IDNS_OrdinaryFriend)) &&
>             "namespace includes other than ordinary or tag");
>
> +    Decl *Prev = getPreviousDecl();
>      IdentifierNamespace = 0;
>      if (OldNS & (IDNS_Tag | IDNS_TagFriend)) {
>        IdentifierNamespace |= IDNS_TagFriend;
> -      if (PreviouslyDeclared) IdentifierNamespace |= IDNS_Tag | IDNS_Type;
> +      if (PerformFriendInjection ||
> +          (Prev && Prev->getIdentifierNamespace() & IDNS_Tag))
> +        IdentifierNamespace |= IDNS_Tag | IDNS_Type;
>      }
>
>      if (OldNS & (IDNS_Ordinary | IDNS_OrdinaryFriend)) {
>        IdentifierNamespace |= IDNS_OrdinaryFriend;
> -      if (PreviouslyDeclared) IdentifierNamespace |= IDNS_Ordinary;
> +      if (PerformFriendInjection ||
> +          (Prev && Prev->getIdentifierNamespace() & IDNS_Ordinary))
> +        IdentifierNamespace |= IDNS_Ordinary;
>      }
>    }
>
>
> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=186199&r1=186198&r2=186199&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Fri Jul 12 15:38:49 2013
> @@ -6328,12 +6328,11 @@ Sema::ActOnFunctionDeclarator(Scope *S,
>      }
>
>      if (isFriend) {
> -      // For now, claim that the objects have no previous declaration.
>        if (FunctionTemplate) {
> -        FunctionTemplate->setObjectOfFriendDecl(false);
> +        FunctionTemplate->setObjectOfFriendDecl();
>          FunctionTemplate->setAccess(AS_public);
>        }
> -      NewFD->setObjectOfFriendDecl(false);
> +      NewFD->setObjectOfFriendDecl();
>        NewFD->setAccess(AS_public);
>      }
>
> @@ -6652,8 +6651,6 @@ Sema::ActOnFunctionDeclarator(Scope *S,
>
>        NewFD->setAccess(Access);
>        if (FunctionTemplate) FunctionTemplate->setAccess(Access);
> -
> -      PrincipalDecl->setObjectOfFriendDecl(true);
>      }
>
>      if (NewFD->isOverloadedOperator() && !DC->isRecord() &&
> @@ -10384,9 +10381,8 @@ CreateNewDecl:
>    // declaration so we always pass true to setObjectOfFriendDecl to make
>    // the tag name visible.
>    if (TUK == TUK_Friend)
> -    New->setObjectOfFriendDecl(/* PreviouslyDeclared = */
> !Previous.empty() ||
> -                               (!FriendSawTagOutsideEnclosingNamespace &&
> -                                getLangOpts().MicrosoftExt));
> +    New->setObjectOfFriendDecl(!FriendSawTagOutsideEnclosingNamespace &&
> +                               getLangOpts().MicrosoftExt);
>
>    // Set the access specifier.
>    if (!Invalid && SearchDC->isRecord())
>
> Modified: cfe/trunk/lib/Sema/SemaLookup.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaLookup.cpp?rev=186199&r1=186198&r2=186199&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaLookup.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaLookup.cpp Fri Jul 12 15:38:49 2013
> @@ -2757,8 +2757,15 @@ void Sema::ArgumentDependentLookup(Decla
>        // If the only declaration here is an ordinary friend, consider
>        // it only if it was declared in an associated classes.
>        if (D->getIdentifierNamespace() == Decl::IDNS_OrdinaryFriend) {
> -        DeclContext *LexDC = D->getLexicalDeclContext();
> -        if (!AssociatedClasses.count(cast<CXXRecordDecl>(LexDC)))
> +        bool DeclaredInAssociatedClass = false;
> +        for (Decl *DI = D; DI; DI = DI->getPreviousDecl()) {
> +          DeclContext *LexDC = DI->getLexicalDeclContext();
> +          if (AssociatedClasses.count(cast<CXXRecordDecl>(LexDC))) {
> +            DeclaredInAssociatedClass = true;
> +            break;
> +          }
> +        }
> +        if (!DeclaredInAssociatedClass)
>            continue;
>        }
>
>
> Modified: cfe/trunk/lib/Sema/SemaTemplate.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaTemplate.cpp?rev=186199&r1=186198&r2=186199&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaTemplate.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaTemplate.cpp Fri Jul 12 15:38:49 2013
> @@ -1120,8 +1120,7 @@ Sema::CheckClassTemplate(Scope *S, unsig
>        NewClass->setAccess(PrevClassTemplate->getAccess());
>      }
>
> -    NewTemplate->setObjectOfFriendDecl(/* PreviouslyDeclared = */
> -                                       PrevClassTemplate != NULL);
> +    NewTemplate->setObjectOfFriendDecl();
>
>      // Friend templates are visible in fairly strange ways.
>      if (!CurContext->isDependentContext()) {
>
> Modified: cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp?rev=186199&r1=186198&r2=186199&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp Fri Jul 12 15:38:49
> 2013
> @@ -960,7 +960,7 @@ Decl *TemplateDeclInstantiator::VisitCla
>      else
>        Inst->setAccess(D->getAccess());
>
> -    Inst->setObjectOfFriendDecl(PrevClassTemplate != 0);
> +    Inst->setObjectOfFriendDecl();
>      // TODO: do we want to track the instantiation progeny of this
>      // friend target decl?
>    } else {
> @@ -1110,8 +1110,8 @@ Decl *TemplateDeclInstantiator::VisitCXX
>
>    // If the original function was part of a friend declaration,
>    // inherit its namespace state.
> -  if (Decl::FriendObjectKind FOK = D->getFriendObjectKind())
> -    Record->setObjectOfFriendDecl(FOK == Decl::FOK_Declared);
> +  if (D->getFriendObjectKind())
> +    Record->setObjectOfFriendDecl();
>
>    // Make sure that anonymous structs and unions are recorded.
>    if (D->isAnonymousStructOrUnion()) {
> @@ -1315,7 +1315,7 @@ Decl *TemplateDeclInstantiator::VisitFun
>      assert(isFriend && "non-friend has dependent specialization info?");
>
>      // This needs to be set now for future sanity.
> -    Function->setObjectOfFriendDecl(/*HasPrevious*/ true);
> +    Function->setObjectOfFriendDecl();
>
>      // Instantiate the explicit template arguments.
>      TemplateArgumentListInfo ExplicitArgs(Info->getLAngleLoc(),
> @@ -1365,13 +1365,7 @@ Decl *TemplateDeclInstantiator::VisitFun
>    // If the original function was part of a friend declaration,
>    // inherit its namespace state and add it to the owner.
>    if (isFriend) {
> -    NamedDecl *PrevDecl;
> -    if (TemplateParams)
> -      PrevDecl = FunctionTemplate->getPreviousDecl();
> -    else
> -      PrevDecl = Function->getPreviousDecl();
> -
> -    PrincipalDecl->setObjectOfFriendDecl(PrevDecl != 0);
> +    PrincipalDecl->setObjectOfFriendDecl();
>      DC->makeDeclVisibleInContext(PrincipalDecl);
>
>      bool queuedInstantiation = false;
> @@ -1639,7 +1633,7 @@ TemplateDeclInstantiator::VisitCXXMethod
>                                                      TemplateParams,
> Method);
>      if (isFriend) {
>        FunctionTemplate->setLexicalDeclContext(Owner);
> -      FunctionTemplate->setObjectOfFriendDecl(true);
> +      FunctionTemplate->setObjectOfFriendDecl();
>      } else if (D->isOutOfLine())
>        FunctionTemplate->setLexicalDeclContext(D->getLexicalDeclContext());
>      Method->setDescribedFunctionTemplate(FunctionTemplate);
> @@ -1666,7 +1660,7 @@ TemplateDeclInstantiator::VisitCXXMethod
>                                              TempParamLists.data());
>
>      Method->setLexicalDeclContext(Owner);
> -    Method->setObjectOfFriendDecl(true);
> +    Method->setObjectOfFriendDecl();
>    } else if (D->isOutOfLine())
>      Method->setLexicalDeclContext(D->getLexicalDeclContext());
>
>
> Modified: cfe/trunk/test/CXX/temp/temp.decls/temp.friend/p4.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/temp/temp.decls/temp.friend/p4.cpp?rev=186199&r1=186198&r2=186199&view=diff
>
> ==============================================================================
> --- cfe/trunk/test/CXX/temp/temp.decls/temp.friend/p4.cpp (original)
> +++ cfe/trunk/test/CXX/temp/temp.decls/temp.friend/p4.cpp Fri Jul 12
> 15:38:49 2013
> @@ -26,3 +26,20 @@ void g() {
>    X2<float> xf;
>    f(xf);
>  }
> +
> +template<typename T>
> +struct X3 {
> +  operator int();
> +
> +  friend void h(int x);
> +};
> +
> +int array2[sizeof(X3<int>)];
> +int array3[sizeof(X3<float>)];
> +
> +void i() {
> +  X3<int> xi;
> +  h(xi);
> +  X3<float> xf;
> +  h(xf);
> +}
>
> Modified: cfe/trunk/test/SemaCXX/friend.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/friend.cpp?rev=186199&r1=186198&r2=186199&view=diff
>
> ==============================================================================
> --- cfe/trunk/test/SemaCXX/friend.cpp (original)
> +++ cfe/trunk/test/SemaCXX/friend.cpp Fri Jul 12 15:38:49 2013
> @@ -163,3 +163,27 @@ namespace test9 {
>      friend void C::f(int, int, int) {}  // expected-error {{no function
> named 'f' with type 'void (int, int, int)' was found in the specified
> scope}}
>    };
>  }
> +
> +namespace test10 {
> +  struct A {
> +    friend void f10();
> +  };
> +  struct B {
> +    friend void f10();
> +  };
> +  void g() {
> +    f10(); // expected-error {{undeclared identifier}}
> +  }
> +}
> +
> +namespace PR16597 {
> +  struct A {
> +    friend void f_16597();
> +  };
> +  struct B {
> +    friend void f_16597();
> +  };
> +  struct C {
> +  };
> +  void g(C a) { f_16597(a); } // expected-error {{undeclared identifier}}
> +}
>
>
> _______________________________________________
> 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/20130715/8ab6f864/attachment.html>


More information about the cfe-commits mailing list