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

endlessroad1991 at gmail.com endlessroad1991 at gmail.com
Wed Dec 12 23:45:39 PST 2012


Thanks, that's a very detailed and thorough comment.

MSDN page for __declspec(property):
http://msdn.microsoft.com/en-us/library/yhfk0thd.aspx
we can declare "array-like" property:
        __declspec(property(get=GetX, put=PutX)) int x[][];
and access it with indices:
        var = obj->x[expr1][expr2];
it will be translated into:
        var = obj->GetX(expr1, expr2);

And that's what "ParamCount" in "PropertyAttr" is for.
We don't know how many params until we parse to "int x[][]", so we have to
modify the Attr when we get here.
Considering this situation, is it still possible to do it in your suggested
way?


On Wed, Dec 12, 2012 at 5:26 AM, John McCall <rjmccall at apple.com> wrote:

> On Dec 10, 2012, at 7:52 PM, "endlessroad1991 at gmail.com" <
> endlessroad1991 at gmail.com> wrote:
> > Hi all, I've rewritten the __declspec(property) implementation,using
> PseudoObject.
> >
> > If you're comfortable with this patch, I'll work on tests, code-style,
> and post it to cfe-commits.
>
> 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.
>
> +class MSPropertyRefExpr : public Expr {
> +private:
> +  MemberExpr *MemberExpr;
> +  ArrayRef<Stmt*> IndexExprs;
> +  SourceLocation StartLoc, EndLoc;
> +  ASTContext& AstContext;
> +  Expr *GetterExpr, *SetterExpr;
> +
>
> 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.
>
> 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.
>
> +def Property : InheritableAttr {
> +  let Spellings = [Declspec<"property">];
> +  let Args = [IntArgument<"ParamCount">,
> +              StringArgument<"Get">,
> +              StringArgument<"Put">];
> +}
>
> Where is ParamCount from?  I don't see this anywhere in the documentation
> for declspec(property).
>
> +  static AttributeList* getPropertyAttr(AttributeList *list) {
> +    while (true) {
> +      if (list == NULL)
> +        return NULL;
> +      if (list->getKind() == AttributeList::AT_Property)
> +        return list;
> +      list = list->getNext();
> +    }
> +  }
>
> This is pretty suspicious.
>
> +    case Expr::MSPropertyRefExprClass:
> +      return Cl::CL_LValue;
>
> What's the effect of doing a property reference on an r-value or x-value?
>
> +void StmtPrinter::VisitMSPropertyRefExpr(MSPropertyRefExpr *Node) {
> +  assert(false);
> +}
> +void StmtProfiler::VisitMSPropertyRefExpr(const clang::MSPropertyRefExpr
> *S) {
> +  assert(false);
> +}
>
> Okay, you had to know that these weren't getting past code review. :)
>
> +    Expr *args[3] = { NULL };
> +    llvm::APInt ii(8, 0);
> +    args[0] = IntegerLiteral::Create(Actions.getASTContext(), ii,
> Actions.getASTContext().UnsignedCharTy, Tok.getLocation());
> +    assert(Tok.getKind() == tok::l_paren);
> +    ConsumeParen();
> +  begin_getter_or_setter:
> +    assert(Tok.getKind() == tok::identifier);
> +    IdentifierInfo *II = Tok.getIdentifierInfo();
> +    assert((II->getName() == "get") || (II->getName() == "put"));
> +    ConsumeToken();
> +    assert(Tok.getKind() == tok::equal);
> +    ConsumeToken();
> +    assert(Tok.getKind() == tok::identifier);
> +    if (II->getName() == "get") {
> +      SourceLocation sl = Tok.getLocation();
> +      args[1] = StringLiteral::Create(Actions.getASTContext(),
> +                                      Tok.getIdentifierInfo()->getName(),
> +                                      StringLiteral::Ascii,
> +                                      false,
> +                                      Actions.getASTContext().VoidPtrTy,
> +                                      &sl, 1);
> +    } else if (II->getName() == "put") {
> +      SourceLocation sl = Tok.getLocation();
> +      args[2] = StringLiteral::Create(Actions.getASTContext(),
> +                                      Tok.getIdentifierInfo()->getName(),
> +                                      StringLiteral::Ascii,
> +                                      false,
> +                                      Actions.getASTContext().VoidPtrTy,
> +                                      &sl, 1);
> +    }
> +    ConsumeToken();
> +    if (Tok.getKind() == tok::comma) {
> +      ConsumeToken();
> +      goto begin_getter_or_setter;
> +    }
> +    assert(Tok.getKind() == tok::r_paren);
> +    ConsumeParen();
> +    Attrs.addNew(Ident, Loc, 0, Loc, 0, Loc, args, 3,
> +                 AttributeList::AS_Declspec);
> +
>
> 1.  You should leave the comment describing the grammar you're parsing
> intact.
> 2.  Please use a do/while loop instead of a label and a goto.
> 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.
> 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.
> 5.  What's going on with this fake integer literal, anyway?
>
> @@ -1558,11 +1559,60 @@ Parser::ParsePostfixExpressionSuffix(ExprResult
> LHS) {
>                                      ObjectType, TemplateKWLoc, Name))
>          LHS = ExprError();
>
> -      if (!LHS.isInvalid())
> -        LHS = Actions.ActOnMemberAccessExpr(getCurScope(), LHS.take(),
> OpLoc,
> +      if (!LHS.isInvalid()) {
> +        Expr *lhs = LHS.take();
> +        LHS = Actions.ActOnMemberAccessExpr(getCurScope(), lhs, OpLoc,
>                                              OpKind, SS, TemplateKWLoc,
> Name,
>                                   CurParsedObjCImpl ?
> CurParsedObjCImpl->Dcl : 0,
>                                              Tok.is(tok::l_paren));
>
> Everything that you're doing in here belongs in Sema instead.
>
> +ExprResult MSPropertyOpBuilder::buildAssignmentOperation(
> +  Scope *Sc, SourceLocation opcLoc,
> +  BinaryOperatorKind opcode, Expr *LHS, Expr *RHS) {
> +  return buildSet(RHS, opcLoc, false);
> +}
> +
> +ExprResult MSPropertyOpBuilder::buildRValueOperation(Expr *op) {
> +  return buildGet();
> +}
>
> You don't need these overrides.
>
> +ExprResult MSPropertyOpBuilder::buildGet() {
> +  SmallVector<Expr*, 12> ArgExprs;
> +  for (unsigned i = 0; i < RefExpr->getIndexExprs().size(); i++) {
> +    Expr * const e = (Expr * const)(RefExpr->getIndexExprs()[i]);
> +    ArgExprs.push_back(e);
> +  }
> +  return S.ActOnCallExpr(S.getCurScope(), RefExpr->getGetterExpr(),
> RefExpr->getSourceRange().getBegin(), ArgExprs,
> RefExpr->getSourceRange().getEnd());
> +}
>
> 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.
>
> I'll leave it at that for now.
>
> John.




-- 
Best Regards, Tong Shen (沈彤)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20121213/65331e96/attachment.html>


More information about the cfe-dev mailing list