[cfe-commits] [patch] Friend semantics

[Douglas Gregor]

Hi John,

On Aug 4, 2009, at 2:49 PM, John McCall wrote:
> This patch implements many, but not all, of the semantics required  
> for friend declarations.  There's a lot left to be done, but it at  
> least gets friend operators passing Sema.

  Very cool.

> I'd appreciate a review, since I'm interacting with lookup / etc. in  
> ways that I haven't touched before.

+def err_introducing_special_friend : Error<
+  "cannot declare a %select{constructor|destructor|conversion  
operator}0"
+  " at namespace scope">;

I find this diagnostic confusing, because the user has written  
something like

	friend operator int() const;

inside a class. From the language perspective, the diagnostic is  
correct. However, I user probably wants to know that he should write a  
nested-name-specifier before the "operator", e.g.,

	friend X::operator int() const;

How about using a diagnostic such as:

   "friend declaration for a %select{constructor|destructor|conversion  
operator}0 should use a qualified name to refer to a member of another  
class"

+Sema::DeclPtrTy Sema::ActOnFriendDecl(Scope *S,
+                       llvm::PointerUnion<const  
DeclSpec*,Declarator*> DU) {

Nice use of llvm::PointerUnion :)

+      if (RD) {
+        Diag(DS.getFriendSpecLoc(), diag::err_unelaborated_friend_type)
+          << (RD->isUnion())
+          << CodeModificationHint::CreateInsertion 
(DS.getTypeSpecTypeLoc(),
+                                      RD->isUnion() ? " union" : "  
class");
+        return DeclPtrTy::make(RD);
+      }

Excellent!

+  SourceLocation Loc = D->getIdentifierLoc();
+  QualType T = GetTypeForDeclarator(*D, S);
+
+  // C++ [class.friend]p1
+  //   A friend of a class is a function or class....
+  if (!T->isFunctionType()) {
+    Diag(Loc, diag::err_unexpected_friend);
+
+    // It might be worthwhile to try to recover by creating an
+    // appropriate declaration.
+    return DeclPtrTy();
    }

Do we need to check whether T is actually a typedef of a function type?

+  // Recover from invalid scope qualifiers as if they just weren't  
there.
+  if (!ScopeQual.isInvalid() && ScopeQual.isSet()) {
+    DC = computeDeclContext(ScopeQual);

computeDeclContext can end up returning NULL, when the scope specifier  
is dependent. For now, we could just exit early in this case and leave  
a FIXME.

+    while (true) {
+      Decl *Dec = LookupQualifiedNameWithType(DC, Name, T);
+      if (Dec) {
+        FD = cast<FunctionDecl>(Dec);
+        break;
+      }
+      if (DC->isFileContext()) break;
+      DC = DC->getParent();
+    }

This should skip over any non-namespace scopes. For example, I suspect  
that this will find Outer::f when it shouldn't:

	struct Outer {
		void f(int);

		struct Inner {
			friend void f(int);
		};
	};			

+    bool Redeclaration = false;
+    NamedDecl *ND = ActOnFunctionDeclarator(S, *D, DC, T,
+                                            /* PrevDecl = */ NULL,
+                                            MultiTemplateParamsArg 
(*this),
+                                            /* isFunctionDef */ false,
+                                            Redeclaration);

We're only doing this when we haven't yet seen the friend declaration,  
and this ends up being a "real" declaration (which we don't want). I  
don't know the right answer, because it depends on the kind of  
representation we pick for friend functions in the AST.

+// Attempts to find a declaration in the given declaration context
+// with exactly the given type.  Returns null if no such declaration
+// was found.
+Decl *Sema::LookupQualifiedNameWithType(DeclContext *DC,
+                                        DeclarationName Name,
+                                        QualType T) {
+  LookupResult result =
+    LookupQualifiedName(DC, Name, LookupOrdinaryName, true);
+
+  for (LookupResult::iterator ir = result.begin(), ie = result.end();
+       ir != ie; ++ir)
+    if (FunctionDecl *CurFD = dyn_cast<FunctionDecl>(*ir))
+      if (CurFD->getType() == T)
+        return CurFD;
+
+  return NULL;
+}
+

I think LookupQualifiedName is going to find more names than we want  
to find. For example, it will look into base classes, which we don't  
need to do. (Or, if we find a result in a base class, we need to  
diagnose it as an error). Here's an example that GCC and EDG both  
reject:

   struct Base {
     void f(int);
   };

   struct Derived : Base { };

   struct Other {
     friend void Derived::f(int);
   };

Also, the type comparison

	if (CurFD->getType() == T)

needs to use canonical types, e.g.,

	if (Context.getCanonicalType(CurFD->getType()) ==  
Context.getCanonicalType(T))

Please go ahead and commit when you're ready.

	- Doug