[cfe-commits] Preliminary patch to support MSVC __declspec(property)
endlessroad1991 at gmail.com
endlessroad1991 at gmail.com
Mon Apr 15 07:36:15 PDT 2013
Sorry, forgot about the patch.
On Mon, Apr 15, 2013 at 10:35 PM, endlessroad1991 at gmail.com <
endlessroad1991 at gmail.com> wrote:
> Rebased on r179523
>
> Replay to John:
>
> On Tue, Apr 9, 2013 at 7:27 AM, John McCall <rjmccall at apple.com> wrote:
>
>> On Apr 1, 2013, at 6:00 AM, endlessroad1991 at gmail.com 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 (沈彤)
>
--
Best Regards, Tong Shen (沈彤)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130415/f0ef07d2/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 1.patch
Type: application/octet-stream
Size: 61092 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130415/f0ef07d2/attachment.obj>
More information about the cfe-commits
mailing list