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

[James Porter]

Douglas Gregor wrote:
> First of all, this patch breaks some tests that are currently working:
> 
[snip]
> 
> We'll need all tests running cleanly before we can commit this change.

I should have these fixed. "make check" is successful, and I ran all the 
tests in SemaCXX and SemaTemplate. Is there a way to automatically run 
these tests along with the ones from "make check"? I ended up just 
writing a shell script to do the ones I mentioned.

> +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.

Fixed, though I didn't see a "getAs" member, so I just used "get" and a 
C-cast. I should probably change that to a static_cast at least, unless 
I'm just mistaken about the existence of "getAs".

> +  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).

I removed this. It seems to work without it anyway.

> 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.

That was just me not fully understanding the usage of CXXScopeSpec. I 
changed the code to what you suggested, and added some checks to make 
sure the qualified-id is valid. It's still a little bit unhappy with 
template code, so I've had to be careful when making it not fail in 
currently-working scenarios.

Aside from eliminating my, er, "naive" code in ActOnMemberReferenceExpr, 
the biggest change is probably in TemplateExprInstantiator:: 
VisitCXXUnresolvedMemberExpr, which now "enters" the class's scope 
before acting on the member. Currently this is just to allow overloaded 
operator-> to work, but it's probably also necessary to get qualified 
name lookups working for templates.

There are a few other places in that file where ActOnMemberReferenceExpr 
is called. I might want to add the ActOnCXX(Enter|Exit)MemberScope 
methods around them too, but they weren't causing any test failures so I 
haven't looked at them too hard yet.

Thanks for taking the time to look over this patch. It's definitely been 
a good learning experience, and I'm getting considerably more 
comfortable with the clang codebase now (though I've probably got a ways 
to go yet!).

- Jim
-------------- next part --------------
A non-text attachment was scrubbed...
Name: qualified-ids.patch
Type: text/x-patch
Size: 15237 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20090801/93cc0dbe/attachment.bin>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: qual-id-test.cpp
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20090801/93cc0dbe/attachment.ksh>