[cfe-dev] WIP patch for qualified-ids in class member access
Douglas Gregor
dgregor at apple.com
Wed Jul 8 19:53:38 PDT 2009
Hi James,
On Jul 6, 2009, at 9:27 PM, James Porter wrote:
> Hi all. I've been following the clang project passively for a little
> while, and figured it was time to start contributing a bit.
Great!
> Currently, the patch does not support qualified-id lookup for
> dependent types or for classes with an overloaded operator->. The
> latter should be straightforward to add, but the former is a bit
> trickier, since I'm not familiar with how clang handles class
> templates yet. There are also likely some bugs with handling
> templates inside the qualified-id, but I haven't had a chance to
> look at that yet. Finally, I haven't handled the actual lookup of
> the members, only the nested-name-specifier part.
Okay. This is a rather non-trivial feature to tackle, so I don't
expect templates to work on the first try.
> Hopefully this patch is more-or-less correct, even though it's not
> complete yet. Comments are, of course, welcome (in fact, they're
> requested!).
>
> - Jim
> Index: include/clang/Parse/Action.h
> ===================================================================
> --- include/clang/Parse/Action.h (revision 74890)
> +++ include/clang/Parse/Action.h (working copy)
> @@ -235,6 +235,14 @@
> return 0;
> }
>
> + virtual bool ActOnCXXEnterMemberScope(Scope *S, ExprArg &exp) {
> + return false;
> + }
> +
> + virtual void ActOnCXXExitMemberScope(Scope *S, ExprArg &exp,
> + CXXScopeSpec &SS) {
> + }
> +
The idea here is that the "exp" argument isn't really being consumed
by ActOnCXXEnterMemberScope or ActOnCXXExitMemberScope, but that "exp"
is there to help these actions enter or exit that scope, right? If so,
I suggest passing an ExprTy* rather than a reference to an ExprArg.
ExprArg is a smart pointer that's meant to be passed by value, so the
signatures above look like a mistake for those of us used to the smart
pointers.
Please add some comments on these two actions, describing their purpose.
> [snip]
> Index: lib/Sema/SemaCXXScopeSpec.cpp
> ===================================================================
> --- lib/Sema/SemaCXXScopeSpec.cpp (revision 74890)
> +++ lib/Sema/SemaCXXScopeSpec.cpp (working copy)
> @@ -14,6 +14,7 @@
> #include "Sema.h"
> #include "clang/AST/ASTContext.h"
> #include "clang/AST/DeclTemplate.h"
> +#include "clang/AST/ExprCXX.h"
> #include "clang/AST/NestedNameSpecifier.h"
> #include "clang/Parse/DeclSpec.h"
> #include "llvm/ADT/STLExtras.h"
> @@ -278,6 +279,65 @@
> T.getTypePtr());
> }
>
> +bool Sema::ActOnCXXEnterMemberScope(Scope *S, ExprArg &Base) {
> + Expr *BaseExpr = static_cast<Expr*>(Base.get());
> + if(BaseExpr == 0)
> + return false;
> +
> + QualType BaseType = BaseExpr->getType();
> + if (BaseExpr->getType()->isPointerType())
> + BaseType = BaseType->getAsPointerType()->getPointeeType();
This will compute a base type even for something like:
class C { int foo; };
void f(C *ptr) {
return ptr.foo;
}
I think that ActOnCXXEnterMemberScope needs to know whether the member
access is with a "->" or a ".", so it knows whether to expect
BaseExpr's type to be a pointer or not. (It's also important when you
get to implementing support for an overloaded operator->).
> + if (BaseType->isDependentType()) {
> + // FIXME: handle dependent types
> + return false;
FIW, just creating a NestedNameSpecifier from the type pointer should
suffice for dependent types (at least for now).
> + } else if (!BaseType->isRecordType()) {
> + Diag(BaseExpr->getExprLoc(),
> + diag::err_typecheck_member_reference_struct_union)
> + << BaseType << BaseExpr->getSourceRange();
> + return false;
> + }
Okay, good.
> + CXXScopeSpec SS;
> + SS.setRange(BaseExpr->getSourceRange());
> + SS.setScopeRep(
> + NestedNameSpecifier::Create(Context, 0, false,
> BaseType.getTypePtr())
> + );
> +
> + ActOnCXXEnterDeclaratorScope(S,SS);
> + return true;
> +}
> +void Sema::ActOnCXXExitMemberScope(Scope *S, ExprArg &Base,
> CXXScopeSpec &SS) {
> + ExitDeclaratorContext(S);
Okay.
> + Expr *BaseExpr = static_cast<Expr*>(Base.get());
> + assert(BaseExpr && "Unable to look up base expression");
> +
> + QualType BaseType = BaseExpr->getType();
> + if (BaseExpr->getType()->isPointerType())
> + BaseType = BaseType->getAsPointerType()->getPointeeType();
> +
> + RecordDecl *Rec = BaseType->getAsRecordType()->getDecl();
It's unfortunate that we have to recompute this base type information :(
> + if (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();
> + DeclarationName Name = T->getAsRecordType()->getDecl()-
> >getDeclName();
> + LookupResult Result = LookupQualifiedName(Rec, Name,
> LookupMemberName,
> + false);
> + if (!Result.getAsDecl()) {
> + Diag(SS.getBeginLoc(), diag::err_not_direct_base_or_virtual)
> + << Name.getAsIdentifierInfo() << BaseType;
> + }
> + }
> +}
This seems a little strange to me. Why isn't this semantic check done
in ActOnMemberExpr?
> +
> /// ActOnCXXEnterDeclaratorScope - Called when a C++ scope specifier
> (global
> /// scope or nested-name-specifier) is parsed, part of a declarator-
> id.
> /// After this method is called, according to [C++ 3.4.3p3], names
> should be
> Index: lib/Sema/SemaExpr.cpp
> ===================================================================
> --- lib/Sema/SemaExpr.cpp (revision 74890)
> +++ lib/Sema/SemaExpr.cpp (working copy)
> @@ -2067,7 +2067,11 @@
> Sema::ActOnMemberReferenceExpr(Scope *S, ExprArg Base,
> SourceLocation OpLoc,
> tok::TokenKind OpKind, SourceLocation
> MemberLoc,
> IdentifierInfo &Member,
> - DeclPtrTy ObjCImpDecl) {
> + DeclPtrTy ObjCImpDecl,const
> CXXScopeSpec *SS) {
> + // FIXME: handle the CXXScopeSpec for proper lookup of qualified-
> ids
> + if (SS && SS->isInvalid())
> + return ExprError();
> +
> Expr *BaseExpr = Base.takeAs<Expr>();
> assert(BaseExpr && "no record expression");
Shouldn't SS be used to perform name lookup for the Member somewhere
in ActOnMemberReferenceExpr?
> Index: lib/Parse/ParseExpr.cpp
> ===================================================================
> --- lib/Parse/ParseExpr.cpp (revision 74890)
> +++ lib/Parse/ParseExpr.cpp (working copy)
> @@ -915,6 +915,14 @@
> tok::TokenKind OpKind = Tok.getKind();
> SourceLocation OpLoc = ConsumeToken(); // Eat the "." or "->"
> token.
>
> + CXXScopeSpec SS;
> + if (getLang().CPlusPlus) {
> + if( !Actions.ActOnCXXEnterMemberScope(CurScope,LHS) )
> + return ExprError();
> + ParseOptionalCXXScopeSpecifier(SS);
> + Actions.ActOnCXXExitMemberScope(CurScope,LHS,SS);
> + }
> +
Oh, this is interesting. I completely expected to have something like:
> + CXXScopeSpec SS;
> + if (getLang().CPlusPlus) {
> + if( !Actions.ActOnCXXEnterMemberScope(CurScope,LHS) )
> + return ExprError();
> + ParseOptionalCXXScopeSpecifier(SS);
followed by a call to ActOnMemberReferenceExpr, then
if (getLang().CPlusPlus)
Actions.ActOnCXXExitMemberScope(CurScope,LHS,SS);
In other words, I expected that we would enter the member scope,
process the member-reference expression within that scope (so that
name lookup would be based on that scope), then exit the member scope
at the end. \
> // RUN: clang-cc -fsyntax-only -verify %s
Thanks for including test cases. You might also consider adding in
some tests that involve templates (even if there are no dependent
types in the expression).
This is a good start, and I think you're on the right track. I think
the biggest question at the moment is how to make ActOnMemberExpr
interact with its CXXScopeSpec parameter.
- Doug
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20090708/6fe1404e/attachment.html>
More information about the cfe-dev
mailing list