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

John McCall rjmccall at apple.com
Mon Apr 15 15:24:40 PDT 2013


On Apr 15, 2013, at 7:35 AM, endlessroad1991 at gmail.com wrote:
> Rebased on r179523

+  bool isDeclspecPropertyAttribute() const  {
+    return IsProperty && getKind() == AT_MsProperty && SyntaxUsed == AS_Declspe
+  }
+

In what ways is this more than just IsProperty?

+  if (DeclSpec::TSCS TSCS = D.getDeclSpec().getThreadStorageClassSpec())
+  Diag(D.getDeclSpec().getThreadStorageClassSpecLoc(),
+       diag::err_invalid_thread)
+    << DeclSpec::getSpecifierName(TSCS);

Indentation.

+  if (II) Loc = D.getIdentifierLoc();

What's with checks like these?  When can you make an anonymous property
declaration?  I think you should just be checking this early and then bailing out
if you don't have a name.

+      
+  case Expr::MSPropertyRefExprClass:
+    return CT_Can;

This case should assert with llvm_unreachable.  If we're processing a bare
MSPropertyRefExpr for whether it can throw, we've done something wrong.

> 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

Ah, sure, you definitely do need to detect this.

> 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?

I think it's fine.
 
> 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.

Sure, good call.

> 
> +  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.

Many of them still have them, but we've come to prefer just befriending the
serialization classes over having tons of getters and setters.

> 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++...

The much better way to handle that is by naming it the right way the first place. :)

> 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"
> }

Ah.  In Sema::ActOnCallExpr, please do this:
  if (Fn->getType() == Context.PseudoObjectTy) {
    ExprResult result = CheckPlaceholderExpr(*this, Fn);
    if (result.isInvalid()) return ExprError();
    Fn = result.take();
  }
right after the isa<CXXPseudoDestructorExpr> block.

And please add this as a test case. :)

> 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 ===

Ah, I think the problem here is that overload resolution and especially
template argument deduction aren't lowering out placeholder types.  Please
hoist this loop out of the C++ block (for obscure reasons; just trust me)
and check:
  hasPlaceholderType(BuiltinType::PseudoObject)
instead of checking for a particular expression kind.

And please add this as a test case. :)

> 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 ===

Ah, good catch.  Yes, this check is exactly right.

And please add this as a test case. :)

In general, this is looking very good, but please add a lot more tests.
You seem to have excellent test coverage;  it would be good to
have some of that in-tree.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130415/a34ce0d0/attachment.html>


More information about the cfe-commits mailing list