<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Hi James,<div><br><div><div>On Jul 6, 2009, at 9:27 PM, James Porter wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>Hi all. I've been following the clang project passively for a little while, and figured it was time to start contributing a bit. </div></blockquote><div><br></div><div>Great!</div><br><blockquote type="cite"><div>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.<br></div></blockquote><div><br></div><div>Okay. This is a rather non-trivial feature to tackle, so I don't expect templates to work on the first try. </div><div><br></div><blockquote type="cite"><div>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!).<br><br>- Jim<br>Index: include/clang/Parse/Action.h<br>===================================================================<br>--- include/clang/Parse/Action.h<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 74890)<br>+++ include/clang/Parse/Action.h<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)<br>@@ -235,6 +235,14 @@<br> return 0; <br> }<br><br>+ virtual bool ActOnCXXEnterMemberScope(Scope *S, ExprArg &exp) {<br>+ return false;<br>+ }<br>+<br>+ virtual void ActOnCXXExitMemberScope(Scope *S, ExprArg &exp,<br>+ CXXScopeSpec &SS) {<br>+ }<br>+<br></div></blockquote><div><br></div><div>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.</div><div><br></div>Please add some comments on these two actions, describing their purpose.</div><div><br><blockquote type="cite"><div>[snip]<br>Index: lib/Sema/SemaCXXScopeSpec.cpp<br>===================================================================<br>--- lib/Sema/SemaCXXScopeSpec.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 74890)<br>+++ lib/Sema/SemaCXXScopeSpec.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)<br>@@ -14,6 +14,7 @@<br> #include "Sema.h"<br> #include "clang/AST/ASTContext.h"<br> #include "clang/AST/DeclTemplate.h"<br>+#include "clang/AST/ExprCXX.h"<br> #include "clang/AST/NestedNameSpecifier.h"<br> #include "clang/Parse/DeclSpec.h"<br> #include "llvm/ADT/STLExtras.h"<br>@@ -278,6 +279,65 @@<br> T.getTypePtr());<br> }<br><br>+bool Sema::ActOnCXXEnterMemberScope(Scope *S, ExprArg &Base) {<br>+ Expr *BaseExpr = static_cast<Expr*>(Base.get());<br>+ if(BaseExpr == 0)<br>+ return false;<br>+<br>+ QualType BaseType = BaseExpr->getType();<br>+ if (BaseExpr->getType()->isPointerType())<br>+ BaseType = BaseType->getAsPointerType()->getPointeeType();<br></div></blockquote><div><br></div><div>This will compute a base type even for something like:</div><div><br></div><div> class C { int foo; };</div><div> void f(C *ptr) {</div><div> return ptr.foo;</div><div> }</div><div><br></div><div>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->).</div><br><blockquote type="cite"><div>+ if (BaseType->isDependentType()) {<br>+ // FIXME: handle dependent types<br>+ return false;<br></div></blockquote><div><br></div>FIW, just creating a NestedNameSpecifier from the type pointer should suffice for dependent types (at least for now).</div><div><br><blockquote type="cite"><div>+ } else if (!BaseType->isRecordType()) {<br>+ Diag(BaseExpr->getExprLoc(), <br>+ diag::err_typecheck_member_reference_struct_union)<br>+ << BaseType << BaseExpr->getSourceRange();<br>+ return false;<br>+ }<br></div></blockquote><div><br></div>Okay, good.</div><div><br><blockquote type="cite"><div>+ CXXScopeSpec SS;<br>+ SS.setRange(BaseExpr->getSourceRange());<br>+ SS.setScopeRep(<br>+ NestedNameSpecifier::Create(Context, 0, false, BaseType.getTypePtr())<br>+ );<br>+<br>+ ActOnCXXEnterDeclaratorScope(S,SS);<br>+ return true;<br>+}<font class="Apple-style-span" color="#000000"><font class="Apple-style-span" color="#144FAE"><br></font></font></div></blockquote><div><br></div><blockquote type="cite"><div>+void Sema::ActOnCXXExitMemberScope(Scope *S, ExprArg &Base, CXXScopeSpec &SS) {<br>+ ExitDeclaratorContext(S);<br></div></blockquote><div><br></div>Okay.</div><div><br><blockquote type="cite"><div>+ Expr *BaseExpr = static_cast<Expr*>(Base.get());<br>+ assert(BaseExpr && "Unable to look up base expression");<br>+<br>+ QualType BaseType = BaseExpr->getType();<br>+ if (BaseExpr->getType()->isPointerType())<br>+ BaseType = BaseType->getAsPointerType()->getPointeeType();<br>+<br>+ RecordDecl *Rec = BaseType->getAsRecordType()->getDecl();<br></div></blockquote><div><br></div>It's unfortunate that we have to recompute this base type information :(</div><div><br></div><div><blockquote type="cite"><div>+ if (SS.isSet()) {<br>+ NestedNameSpecifier *Prefix<br>+ = static_cast<NestedNameSpecifier *>(SS.getScopeRep());<br>+ assert(Prefix && Prefix->getKind() == NestedNameSpecifier::TypeSpec<br>+ && "Nested name specifier does not name a type");<br>+<br>+ Type *T = Prefix->getAsType();<br>+ DeclarationName Name = T->getAsRecordType()->getDecl()->getDeclName();<br>+ LookupResult Result = LookupQualifiedName(Rec, Name, LookupMemberName,<br>+ false);<br>+ if (!Result.getAsDecl()) {<br>+ Diag(SS.getBeginLoc(), diag::err_not_direct_base_or_virtual)<br>+ << Name.getAsIdentifierInfo() << BaseType;<br>+ }<br>+ }<br>+}<br></div></blockquote><div><br></div><div>This seems a little strange to me. Why isn't this semantic check done in ActOnMemberExpr? </div><br><blockquote type="cite"><div>+<br> /// ActOnCXXEnterDeclaratorScope - Called when a C++ scope specifier (global<br> /// scope or nested-name-specifier) is parsed, part of a declarator-id.<br> /// After this method is called, according to [C++ 3.4.3p3], names should be<br>Index: lib/Sema/SemaExpr.cpp<br>===================================================================<br>--- lib/Sema/SemaExpr.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 74890)<br>+++ lib/Sema/SemaExpr.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)<br>@@ -2067,7 +2067,11 @@<br> Sema::ActOnMemberReferenceExpr(Scope *S, ExprArg Base, SourceLocation OpLoc,<br> tok::TokenKind OpKind, SourceLocation MemberLoc,<br> IdentifierInfo &Member,<br>- DeclPtrTy ObjCImpDecl) {<br>+ DeclPtrTy ObjCImpDecl,const CXXScopeSpec *SS) {<br>+ // FIXME: handle the CXXScopeSpec for proper lookup of qualified-ids<br>+ if (SS && SS->isInvalid())<br>+ return ExprError();<br>+<br> Expr *BaseExpr = Base.takeAs<Expr>();<br> assert(BaseExpr && "no record expression");<br></div></blockquote><div><br></div><div>Shouldn't SS be used to perform name lookup for the Member somewhere in ActOnMemberReferenceExpr?</div><br><blockquote type="cite"><div>Index: lib/Parse/ParseExpr.cpp<br>===================================================================<br>--- lib/Parse/ParseExpr.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 74890)<br>+++ lib/Parse/ParseExpr.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)<br>@@ -915,6 +915,14 @@<br> tok::TokenKind OpKind = Tok.getKind();<br> SourceLocation OpLoc = ConsumeToken(); // Eat the "." or "->" token.<br><br>+ CXXScopeSpec SS;<br>+ if (getLang().CPlusPlus) {<br>+ if( !Actions.ActOnCXXEnterMemberScope(CurScope,LHS) )<br>+ return ExprError();<br>+ ParseOptionalCXXScopeSpecifier(SS);<br>+ Actions.ActOnCXXExitMemberScope(CurScope,LHS,SS);<br>+ }<br>+<br></div></blockquote><div><br></div><div>Oh, this is interesting. I completely expected to have something like:</div><div><br></div><div><br></div><blockquote type="cite"><div>+ CXXScopeSpec SS;<br>+ if (getLang().CPlusPlus) {<br>+ if( !Actions.ActOnCXXEnterMemberScope(CurScope,LHS) )<br>+ return ExprError();<br>+ ParseOptionalCXXScopeSpecifier(SS);</div></blockquote><div><br></div>followed by a call to ActOnMemberReferenceExpr, then</div><div><br></div><div> if (getLang().CPlusPlus)</div><div> Actions.ActOnCXXExitMemberScope(CurScope,LHS,SS);</div><div><br></div><div>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. \</div><div><br><blockquote type="cite"><div>// RUN: clang-cc -fsyntax-only -verify %s <br></div></blockquote><br><div>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).</div></div><div><br></div><div>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.</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>- Doug</div></div></body></html>