Rebased on r179523 Replay to John:
On Tue, Apr 9, 2013 at 7:27 AM, John McCall <[email protected]> wrote: > On Apr 1, 2013, at 6:00 AM, [email protected] wrote: > > Rebased on r178459 > > > > All original tests still pass, and 2 new tests(one for Parser, one for > Sema). > > Major cross-cutting problem: none of the places where you're manually > calling CheckPlaceholderExpr should be necessary. I'm positive that > we're already filtering out placeholders in these places; if we weren't, > we'd have a serious bug that would hit a ton of Objective-C code. > > The other nice thing about relying on the existing machinery for this is > that you wouldn't have a ton of latent bugs about not looking through > parens. :) > Yes, I'm aware of that :-) I will explain each case of CheckPlaceholderExpr() in the last. > > SemaTemplateInstantiateDecl.cpp: > > + if (DI->getType()->isInstantiationDependentType() || > + DI->getType()->isVariablyModifiedType()) { > > Why are you checking for variably-modified types here? > I just copied most of handleField() into handleMSProperty(), In the attached new patch, it will complain about VariablyModifiedType > > + if (CXXRecordDecl *Parent = > + dyn_cast<CXXRecordDecl>(Property->getDeclContext())) { > + if (Parent->isAnonymousStructOrUnion() && > + Parent->getRedeclContext()->isFunctionOrMethod()) > + SemaRef.CurrentInstantiationScope->InstantiatedLocal(D, Property); > + } > > There should be an invariant that the parent of an MSPropertyDecl is > a CXXRecordDecl. > Done. This "if" is removed. > > SemaDecl.cpp: > > This is all C++-specific and should go in SemaDeclCXX.cpp. > Done. Moved to SemaDeclCXX.cpp > > + DiagnoseFunctionSpecifiers(D.getDeclSpec()); > + > + if (D.getDeclSpec().isThreadSpecified()) > + Diag(D.getDeclSpec().getThreadSpecLoc(), diag::err_invalid_thread); > + if (D.getDeclSpec().isConstexprSpecified()) > + Diag(D.getDeclSpec().getConstexprSpecLoc(), > diag::err_invalid_constexpr) > + << 2; > > You should also complain about the presence of a storage class specifier. > Done. > > + MSPropertyDecl *NewFD; > > Style nit: "NewPD" instead of "NewFD". > Done. > > + llvm::StringRef GetterName = MSPropertyAttr->getScopeName() ? > + MSPropertyAttr->getScopeName()->getName() : StringRef(); > + llvm::StringRef SetterName = MSPropertyAttr->getParameterName() ? > + MSPropertyAttr->getParameterName()->getName() : StringRef(); > > Why are you storing getter/setter names as a StringRefs instead of an > IdentifierInfo*? Identifiers are (1) more compact, (2) better supported by > the serialization libraries, and (3) required by lookup anyway. > Done. And this does make the code much cleaner! I should have realized it before. > > SemaPseudoObject.cpp: > > + } else if (MSPropertyRefExpr *refExpr > + = dyn_cast<MSPropertyRefExpr>(opaqueRef)) { > + return refExpr; > > You need to implement this correctly; you should follow the lead > of the existing implementations. > Done. > > SemaDeclCXX.cpp: > > +static AttributeList *getMSPropertyAttr(AttributeList *list) { > + for (AttributeList* it = list; it != 0; it = it->getNext()) > + if (it->getName()->getName() == "property") > > if (it->getKind() == AttributeList::AT_property) > > I'm a little bothered by doing this here instead of filtering it out > beforehand, > but it's probably okay; attributes on fields are relatively uncommon. > I don't like this either, but ParsedAttr are only attached to FieldDecl after it's created... And here, we need to judge whether we should create FieldDecl or MSPropertyDecl. So I just put it here. Any ideas? > > AttributeList.cpp: > > - if (ScopeName) > - Buf += ScopeName->getName(); > - // Ensure that in the case of C++11 attributes, we look for '::foo' if > it is > - // unscoped. > - if (ScopeName || SyntaxUsed == AS_CXX11) > - Buf += "::"; > + // For property, "ScopeName" is used to store getter function name > + if (AttrName != "property") { > + if (ScopeName) > + Buf += ScopeName->getName(); > + // Ensure that in the case of C++11 attributes, we look for '::foo' > if it is > + // unscoped. > + if (ScopeName || SyntaxUsed == AS_CXX11) > + Buf += "::"; > + } > > Okay, this is actually a hot enough path that we'd really rather not > introduce > a string compare into it. Figure out some other way to store the second > string; you could throw things like MessageExpr into a union. > Done. Turns out that this problem has been encountered before: availability and typetag attributes. I used the same mechanism as existing code: malloc more than sizeof(AttributeList), and use the extra space. > > ASTBitCodes.h: > > + /// \brief A __delcpec(property) record. > + DECL_MS_PROPERTY, > > By prevailing style in this file, this should be "An MSPropertyDecl > record." > Done. > > ExprCXX.h: > > + SourceLocation MemberLoc; > + DeclarationNameLoc MemberDNLoc; > + bool IsArrow; > > Please put the bool immediately after the SourceLocation; it'll pack > better > on 64-bit architectures. > Done. > > + SourceLocation getLocStart() const { > + if (QualifierLoc) { > + return QualifierLoc.getBeginLoc(); > + } else { > + SourceLocation result = BaseExpr->getLocStart(); > + if (result.isValid()) > + return result; > + else > + return MemberLoc; > + } > + } > > You've got this mixed up. The start loc is the start of the base > expression, unless that's an implicit this access, in which case it's the > start of the qualifier, unless that doesn't exist, in which case it's the > member loc. > > You should have an isImplicitAccess() method like MemberExpr does. > Done. > > + Expr* getBaseExpr() const { return BaseExpr; } > + void setBaseExpr(Expr *baseExpr) { BaseExpr = baseExpr; } > + MSPropertyDecl* getMSPropertyDecl() const { return Decl; } > + void setMSPropertyDecl(MSPropertyDecl *decl) { Decl = decl; } > + bool isArrow() const { return IsArrow; } > + void setArrow(bool isArrow) { IsArrow = isArrow; } > + SourceLocation getMemberLoc() const { return MemberLoc; } > + void setMemberLoc(SourceLocation memberLoc) { MemberLoc = memberLoc; } > + DeclarationNameLoc getMemberDNLoc() const { return MemberDNLoc; } > + void setMemberDNLoc(DeclarationNameLoc memberDNLoc) { > + MemberDNLoc = memberDNLoc; > + } > + NestedNameSpecifierLoc getQualifierLoc() const { return QualifierLoc; } > + void setQualifierLoc(NestedNameSpecifierLoc qualifierLoc) { > + QualifierLoc = qualifierLoc; > + } > > The statement/expression AST classes are generally meant to be immutable, > and there's really no need to have any of these setters. > Well, these setter's are for serialization. Every subclass of Expr has them... So they should be kept. > > Attr.td: > > +def MsProperty : InheritableAttr { > + let Spellings = [Declspec<"property">]; > +} > + > > It's not actually Inheritable; properties can't be redeclared. > Done. > > test/Parser/MicrosoftExtensions.c: > > Why is this in a C test case? Is this actually legal in C? I can't > imagine > it is, because it the entire language model relies on methods. > > You can parse the attribute in C, I guess, but you should always emit an > error if you see it. > On top of this test file, it says "-x objective-c++". So this file is treated as C++... > > +struct S8 { > + __declspec(property) int V0; /* expected-error {{expected '(' > after 'property'}} */ > + __declspec(property()) int V1; /* expected-error {{expected 'get' > or 'put' in __declspec(property)}} expected-error {{no getter or putter in > __declspec(property)}} */ > + __declspec(property(set)) int V2; /* expected-error {{expected > 'get' or 'put' in __declspec(property)}} expected-error {{no getter or > putter in __declspec(property)}} */ > + __declspec(property(get)) int V3; /* expected-error {{expected '=' > in __declspec(property)}} expected-error {{no getter or putter in > __declspec(property)}} */ > + __declspec(property(get&)) int V4; /* expected-error {{expected > '=' in __declspec(property)}} expected-error {{no getter or putter in > __declspec(property)}} */ > + __declspec(property(get=)) int V5; /* expected-error {{expected > identifier in __declspec(property)}} expected-error {{no getter or putter > in __declspec(property)}} */ > + __declspec(property(get=GetV)) int V6; /* no-warning */ > + __declspec(property(get=GetV=)) int V7; /* expected-error > {{expected ',' or ')' in __declspec(property)}} */ > + __declspec(property(get=GetV,)) int V8; /* expected-error > {{expected 'get' or 'put' in __declspec(property)}} */ > + __declspec(property(get=GetV,put=SetV)) int V9; /* no-warning */ > + __declspec(property(get=GetV,put=SetV,get=GetV)) int V10; /* > expected-error {{duplicate accessor in __declspec(property)}} */ > +}; > > You do not have a single test case that verifies that you can actually > access one of these. > Added. > > John. > Explanation for all CheckPlaceholderExpr(): 1. in Parse/ParseExpr.cpp, ParsePostfixExpressionSuffix() === Code Start === class D { public: bool operator()() { return true; } }; class C { public: __declspec(property(get=GetV)) D V; D GetV() { return D(); } }; int main() { C c; bool b = c.V(); // If not added, clang will complain "c.V is not a function, nor function object" } === Code End === 2. In Sema/SemaExpr.cpp, ActOnCallExpr(): I admit that I'm not very thorough about this one... But here the CheckPlaceholderExpr() call does solve the follwing problem. === Code Start === // If compiled with -ggdb, clang will crash and saying something about // "unexpected builtin-type" in some CG code. struct S2 { template <class T> void f(T t) {} }; template <class T> struct S { __declspec(property(get=GetV)) int V; int GetV() { return 123; } void f() { S2 s2; s2.f(V); } }; int main() { S<int> s; s.f(); } === Code End === 3. In Sema/SemaExprCXX.cpp, BuildCXXNew() === Code Start === template <class T> class C { public: __declspec(property(get=GetV)) T V; int GetV() { return 123; } void f() { int *p = new int[V]; } // If not added, clang will complain "V is dependent type, not integral type" }; int main() { } === Code End === 4. In Sema/SemaDecl.cpp, AddInitializerToDecl() === Code Start === template <class T> class C { public: __declspec(property(get=GetV)) T V; T GetV() { return 123; } void f() { int t = V; } // If not added, clang will complain "V is dependent type" }; int main() { C<int> c; c.f(); } === Code End === -- Best Regards, Tong Shen (沈彤)
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
