[cfe-dev] WIP patch for qualified-ids in class member access

[Douglas Gregor]

On Jul 26, 2009, at 12:17 PM, James Porter wrote:

> James Porter wrote:
>> I've updated my patch (attached). The main change is that this  
>> version
>> supports qualified-ids with overloaded operator->. Other than that,  
>> most
>> of the changes are merely organizational.
>
> Ping! If anyone has a chance to look over this to evaluate the changes
> to this patch, that'd be excellent. I think I ended up posting it  
> right
> around when all the C++ folks were busy in Germany at the Frankfurt  
> meeting.

Your timing was perfect; I didn't even realize you'd posted a updated  
patch :)

Some comments:

First of all, this patch breaks some tests that are currently working:

   test/SemaCXX/class.cpp
   test/SemaCXX/member-expr.cpp
   test/SemaTemplate/ext-vector-type.cpp
   test/SemaTemplate/instantiate-clang.cpp
   test/SemaTemplate/instantiate-expr-4.cpp

We'll need all tests running cleanly before we can commit this change.

+Action::OwningExprResult
+Sema::ActOnCXXEnterMemberScope(Scope *S, CXXScopeSpec &SS, ExprArg  
Base,
+                               tok::TokenKind OpKind) {
+  Expr *BaseExpr = Base.takeAs<Expr>();
+  assert(BaseExpr && "no record expansion");

At this point, nobody "owns" the expression BaseExpr. So, if we return  
without explicitly destroying it, we'll leak the expression. I'd  
suggest letting Base own the expression. Then, code like this:

+  if (BaseType->isDependentType())
+    return Owned(BaseExpr);

can just move Base out to the result, e.g.,

   if (BaseType->isDependentType())
     return move(Base);

Then, when coping with an overloaded operator->:

+      Base = BuildOverloadedArrowExpr(S, BaseExpr, BaseExpr- 
 >getExprLoc());
+      BaseExpr = Base.takeAs<Expr>();
+      if (BaseExpr == NULL)
+          return ExprError();

Replace the "takeAs" with "getAs", so we don't take ownership from  
Base. If Base always owns the expression, it'll always clean it up  
properly.

+  if (!BaseType->isRecordType()) {
+    return ExprError(Diag(BaseExpr->getExprLoc(),
+                           
diag::err_typecheck_member_reference_struct_union)
+                     << BaseType << BaseExpr->getSourceRange());
+  }

We can also end up with various Objective-C types and ExtVectorTypes  
here, so this diagnostic comes too early. (That's why SemaTemplate/ext- 
vector-type.cpp is failing).

+    // FIXME: we should probably do something with the LookupResult
+    if (SS && SS->isSet()) {
+      NestedNameSpecifier *Prefix
+        = static_cast<NestedNameSpecifier *>(SS->getScopeRep());
+      assert(Prefix && Prefix->getKind() ==  
NestedNameSpecifier::TypeSpec
+             && "Nested name specifier does not name a type");
+
+      Type *T = Prefix->getAsType();
+      assert(T && "couldn't get type");
+      DeclarationName Name = T->getAsRecordType()->getDecl()- 
 >getDeclName();
+      LookupResult Result = LookupQualifiedName(RDecl, Name,  
LookupMemberName,
+                                                false);
+      if (!Result.getAsDecl())
+        return ExprError(Diag(SS->getBeginLoc(),
+                              diag::err_not_direct_base_or_virtual)
+                         << Name.getAsIdentifierInfo() << BaseType);
+    }
+
      // The record definition is complete, now make sure the member  
is valid.
      // FIXME: Qualified name lookup for C++ is a bit more  
complicated than this.
      LookupResult Result

I don't really understand what the lookup above is meant to do. I  
expected that it would take the nested-name-specifier in SS and try to  
find the member name (Member) within that scope-specifier, but instead  
it's looking for the name in the nested-name-specifier itself (?)

I had expected that the lookup for the member would do something like  
this:

    // Perform name lookup for the member
    DeclContext *DC = RDecl;
    if (SS && SS->isSet()) {
      // If the member name was a qualified-id, look into the
      // nested-name-specifier.
      DC = computeDeclContext(*SS, false);
      assert(DC && "Cannot handle non-computable dependent contexts in  
lookup");
    }

      LookupResult Result
        = LookupQualifiedName(DC, DeclarationName(&Member),  
LookupMemberName, false);

That would replace the current LookupQualifiedName call, which always  
looks into RDecl.

	- Doug