<div>Thanks, that's a very detailed and thorough comment.</div><div> </div><div>MSDN page for __declspec(property): <a href="http://msdn.microsoft.com/en-us/library/yhfk0thd.aspx">http://msdn.microsoft.com/en-us/library/yhfk0thd.aspx</a></div>
<div>we can declare "array-like" property:</div><div>        __declspec(property(get=GetX, put=PutX)) int x[][];</div><div>and access it with indices:</div><div>        var = obj->x[expr1][expr2];</div><div>it will be translated into:</div>
<div>        var = obj->GetX(expr1, expr2);</div><div> </div><div>And that's what "ParamCount" in "PropertyAttr" is for.</div><div>We don't know how many params until we parse to "int x[][]", so we have to modify the Attr when we get here.</div>
<div>Considering this situation, is it still possible to do it in your suggested way?</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Dec 12, 2012 at 5:26 AM, John McCall <span dir="ltr"><<a href="mailto:rjmccall@apple.com" target="_blank">rjmccall@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Dec 10, 2012, at 7:52 PM, "<a href="mailto:endlessroad1991@gmail.com">endlessroad1991@gmail.com</a>" <<a href="mailto:endlessroad1991@gmail.com">endlessroad1991@gmail.com</a>> wrote:<br>

> Hi all, I've rewritten the __declspec(property) implementation,using PseudoObject.<br>
><br>
> If you're comfortable with this patch, I'll work on tests, code-style, and post it to cfe-commits.<br>
<br>
</div>Posting to cfe-commits is appropriate for detailed review anyway.  I've left cfe-dev CC'ed, but please remove it as a CC if you reply.<br>
<br>
+class MSPropertyRefExpr : public Expr {<br>
+private:<br>
+  MemberExpr *MemberExpr;<br>
+  ArrayRef<Stmt*> IndexExprs;<br>
+  SourceLocation StartLoc, EndLoc;<br>
+  ASTContext& AstContext;<br>
+  Expr *GetterExpr, *SetterExpr;<br>
+<br>
<br>
Even just looking at these members, something seems very wrong here.  Why does an Expr need an ASTContext reference?  Why are the getter and setter expressions recorded here instead of being lazily derived as part of the pseudo-object processing?  You're also losing source information about the index expressions — in particular, the bracket ranges, but also any parens structure that might have appeared in the source.<br>

<br>
Talking with Doug about it, I think the most reasonable thing here is to not have a special MSPropertyRefExpr, and instead to have the AST exactly mirror what you'd get from "primitive" member references and subscripts:  a MemberExpr as the base, possibly surrounded by ArraySubscriptExprs.  Normally that kind of re-use for a different semantic purpose would be a serious faux pas, but because the PseudoObjectExpr isolates these within the syntactic form, it's okay — better than okay, really, because all sorts of code doesn't have to be duplicated for the new expression nodes that look syntactically exactly like the old ones.  Of course, all of these syntactic-form expressions would still have PseudoObjectTy.<br>

<br>
+def Property : InheritableAttr {<br>
+  let Spellings = [Declspec<"property">];<br>
+  let Args = [IntArgument<"ParamCount">,<br>
+              StringArgument<"Get">,<br>
+              StringArgument<"Put">];<br>
+}<br>
<br>
Where is ParamCount from?  I don't see this anywhere in the documentation for declspec(property).<br>
<br>
+  static AttributeList* getPropertyAttr(AttributeList *list) {<br>
+    while (true) {<br>
+      if (list == NULL)<br>
+        return NULL;<br>
+      if (list->getKind() == AttributeList::AT_Property)<br>
+        return list;<br>
+      list = list->getNext();<br>
+    }<br>
+  }<br>
<br>
This is pretty suspicious.<br>
<br>
+    case Expr::MSPropertyRefExprClass:<br>
+      return Cl::CL_LValue;<br>
<br>
What's the effect of doing a property reference on an r-value or x-value?<br>
<br>
+void StmtPrinter::VisitMSPropertyRefExpr(MSPropertyRefExpr *Node) {<br>
+  assert(false);<br>
+}<br>
+void StmtProfiler::VisitMSPropertyRefExpr(const clang::MSPropertyRefExpr *S) {<br>
+  assert(false);<br>
+}<br>
<br>
Okay, you had to know that these weren't getting past code review. :)<br>
<br>
+    Expr *args[3] = { NULL };<br>
+    llvm::APInt ii(8, 0);<br>
+    args[0] = IntegerLiteral::Create(Actions.getASTContext(), ii, Actions.getASTContext().UnsignedCharTy, Tok.getLocation());<br>
+    assert(Tok.getKind() == tok::l_paren);<br>
+    ConsumeParen();<br>
+  begin_getter_or_setter:<br>
+    assert(Tok.getKind() == tok::identifier);<br>
+    IdentifierInfo *II = Tok.getIdentifierInfo();<br>
+    assert((II->getName() == "get") || (II->getName() == "put"));<br>
+    ConsumeToken();<br>
+    assert(Tok.getKind() == tok::equal);<br>
+    ConsumeToken();<br>
+    assert(Tok.getKind() == tok::identifier);<br>
+    if (II->getName() == "get") {<br>
+      SourceLocation sl = Tok.getLocation();<br>
+      args[1] = StringLiteral::Create(Actions.getASTContext(),<br>
+                                      Tok.getIdentifierInfo()->getName(),<br>
+                                      StringLiteral::Ascii,<br>
+                                      false,<br>
+                                      Actions.getASTContext().VoidPtrTy,<br>
+                                      &sl, 1);<br>
+    } else if (II->getName() == "put") {<br>
+      SourceLocation sl = Tok.getLocation();<br>
+      args[2] = StringLiteral::Create(Actions.getASTContext(),<br>
+                                      Tok.getIdentifierInfo()->getName(),<br>
+                                      StringLiteral::Ascii,<br>
+                                      false,<br>
+                                      Actions.getASTContext().VoidPtrTy,<br>
+                                      &sl, 1);<br>
+    }<br>
+    ConsumeToken();<br>
+    if (Tok.getKind() == tok::comma) {<br>
+      ConsumeToken();<br>
+      goto begin_getter_or_setter;<br>
+    }<br>
+    assert(Tok.getKind() == tok::r_paren);<br>
+    ConsumeParen();<br>
+    Attrs.addNew(Ident, Loc, 0, Loc, 0, Loc, args, 3,<br>
+                 AttributeList::AS_Declspec);<br>
+<br>
<br>
1.  You should leave the comment describing the grammar you're parsing intact.<br>
2.  Please use a do/while loop instead of a label and a goto.<br>
3.  Making a pair of string literal expressions just to get this through AttributeList is absurd.  Feel free to re-use the "scope" and "parm name" fields as long as you document the hack.<br>
4.  You cannot actually *assert* that the attribute is grammatically well-formed;  you actually need to check this and emit an error if you don't see the right thing.  You should also add testcases with ill-formed attributes.<br>

5.  What's going on with this fake integer literal, anyway?<br>
<br>
@@ -1558,11 +1559,60 @@ Parser::ParsePostfixExpressionSuffix(ExprResult LHS) {<br>
                                     ObjectType, TemplateKWLoc, Name))<br>
         LHS = ExprError();<br>
<br>
-      if (!LHS.isInvalid())<br>
-        LHS = Actions.ActOnMemberAccessExpr(getCurScope(), LHS.take(), OpLoc,<br>
+      if (!LHS.isInvalid()) {<br>
+        Expr *lhs = LHS.take();<br>
+        LHS = Actions.ActOnMemberAccessExpr(getCurScope(), lhs, OpLoc,<br>
                                             OpKind, SS, TemplateKWLoc, Name,<br>
                                  CurParsedObjCImpl ? CurParsedObjCImpl->Dcl : 0,<br>
                                             Tok.is(tok::l_paren));<br>
<br>
Everything that you're doing in here belongs in Sema instead.<br>
<br>
+ExprResult MSPropertyOpBuilder::buildAssignmentOperation(<br>
+  Scope *Sc, SourceLocation opcLoc,<br>
+  BinaryOperatorKind opcode, Expr *LHS, Expr *RHS) {<br>
+  return buildSet(RHS, opcLoc, false);<br>
+}<br>
+<br>
+ExprResult MSPropertyOpBuilder::buildRValueOperation(Expr *op) {<br>
+  return buildGet();<br>
+}<br>
<br>
You don't need these overrides.<br>
<br>
+ExprResult MSPropertyOpBuilder::buildGet() {<br>
+  SmallVector<Expr*, 12> ArgExprs;<br>
+  for (unsigned i = 0; i < RefExpr->getIndexExprs().size(); i++) {<br>
+    Expr * const e = (Expr * const)(RefExpr->getIndexExprs()[i]);<br>
+    ArgExprs.push_back(e);<br>
+  }<br>
+  return S.ActOnCallExpr(S.getCurScope(), RefExpr->getGetterExpr(), RefExpr->getSourceRange().getBegin(), ArgExprs, RefExpr->getSourceRange().getEnd());<br>
+}<br>
<br>
You should be building the getter expression here instead of trying to build it immediately upon parsing the member expression.  For example, there may not be a getter or setter, or they might not be accessible in this context — you don't want to diagnose these problems unless you actually need to.<br>

<br>
I'll leave it at that for now.<br>
<span class="HOEnZb"><font color="#888888"><br>
John.</font></span></blockquote></div><br><br clear="all"><br>-- <br>Best Regards, Tong Shen (沈彤)<br>
</div>