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

[John McCall]

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. :)

SemaTemplateInstantiateDecl.cpp:

+  if (DI->getType()->isInstantiationDependentType() ||
+      DI->getType()->isVariablyModifiedType())  {

Why are you checking for variably-modified types here?

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

SemaDecl.cpp:

This is all C++-specific and should go in 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.

+  MSPropertyDecl *NewFD;

Style nit:  "NewPD" instead of "NewFD".

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

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.

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.

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.

ASTBitCodes.h:

+      /// \brief A __delcpec(property) record.
+      DECL_MS_PROPERTY,

By prevailing style in this file, this should be "An MSPropertyDecl record."

ExprCXX.h:

+  SourceLocation MemberLoc;
+  DeclarationNameLoc MemberDNLoc;
+  bool IsArrow;

Please put the bool immediately after the SourceLocation;  it'll pack better
on 64-bit architectures.

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

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

Attr.td:

+def MsProperty : InheritableAttr {
+  let Spellings = [Declspec<"property">];
+}
+

It's not actually Inheritable;  properties can't be redeclared.

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.

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

John.