[cfe-commits] Preliminary patch to support MSVC __declspec(property)

[endlessroad1991 at gmail.com]

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 (沈彤)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130415/c3012248/attachment.html>