[PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.
ABataev created this revision. ABataev added reviewers: rnk, rjmccall. ABataev added a subscriber: cfe-commits. All problems described in http://llvm.org/PR25636 are implemented except for return value of the 'put' property. This patch fixes this problem with the indexed properties http://reviews.llvm.org/D15174 Files: lib/Sema/SemaPseudoObject.cpp test/CodeGenCXX/ms-property.cpp test/SemaCXX/ms-property-error.cpp test/SemaCXX/ms-property.cpp Index: lib/Sema/SemaPseudoObject.cpp === --- lib/Sema/SemaPseudoObject.cpp +++ lib/Sema/SemaPseudoObject.cpp @@ -228,6 +228,11 @@ ResultIndex = Semantics.size() - 1; } +void setResultToNextSemantic() { + assert(ResultIndex == PseudoObjectExpr::NoResult); + ResultIndex = Semantics.size(); +} + /// Return true if assignments have a non-void result. bool CanCaptureValue(Expr *exp) { if (exp->isGLValue()) @@ -1489,12 +1494,21 @@ return ExprError(); } - SmallVector ArgExprs; + SmallVector ArgExprs; ArgExprs.append(CallArgs.begin(), CallArgs.end()); ArgExprs.push_back(op); - return S.ActOnCallExpr(S.getCurScope(), SetterExpr.get(), - RefExpr->getSourceRange().getBegin(), ArgExprs, - op->getSourceRange().getEnd()); + ExprResult Res = S.ActOnCallExpr(S.getCurScope(), SetterExpr.get(), + RefExpr->getSourceRange().getBegin(), + ArgExprs, op->getSourceRange().getEnd()); + + if (!Res.isInvalid() && captureSetValueAsResult) { +Expr *ResExpr = Res.get(); +if (!ResExpr->getType()->isVoidType() && +(ResExpr->getType()->isDependentType() || CanCaptureValue(ResExpr))) + setResultToNextSemantic(); + } + + return Res; } //===--===// Index: test/SemaCXX/ms-property.cpp === --- test/SemaCXX/ms-property.cpp +++ test/SemaCXX/ms-property.cpp @@ -29,7 +29,7 @@ public: __declspec(property(get=GetX,put=PutX)) T x[]; T GetX(T i, T j) { return i+j; } - void PutX(T i, T j, T k) { j = i = k; } + T PutX(T i, T j, T k) { return j = i = k; } ~St() { x[0][0] = x[1][1]; } }; @@ -52,6 +52,8 @@ ((p2->x)[23])[1] = j1; // CHECK-NEXT: ++(((p2->x)[23])[1]); ++(((p2->x)[23])[1]); + // CHECK-NEXT: j1 = ((p2->x)[23])[1] = j1; + j1 = ((p2->x)[23])[1] = j1; // CHECK-NEXT: return Test1::GetTest1()->X; return Test1::GetTest1()->X; } Index: test/SemaCXX/ms-property-error.cpp === --- test/SemaCXX/ms-property-error.cpp +++ test/SemaCXX/ms-property-error.cpp @@ -7,17 +7,19 @@ void PutX(int i, int j, int k) { j = i = k; } // expected-note {{'PutX' declared here}} }; +char *ptr; template class St { public: __declspec(property(get=GetX,put=PutX)) T x[]; T GetX(T i, T j) { return i+j; } // expected-note 3 {{'GetX' declared here}} - void PutX(T i, T j, T k) { j = i = k; } // expected-note 2 {{'PutX' declared here}} + T PutX(T i, T j, T k) { return j = i = k; } // expected-note 2 {{'PutX' declared here}} ~St() { x[1] = 0; // expected-error {{too few arguments to function call, expected 3, have 2}} x[2][3] = 4; ++x[2][3]; x[1][2] = x[3][4][5]; // expected-error {{too many arguments to function call, expected 2, have 3}} +ptr = x[1][2] = x[3][4]; // expected-error {{assigning to 'char *' from incompatible type 'int'}} } }; @@ -30,5 +32,6 @@ (p1->x[23]) = argc; // expected-error {{too few arguments to function call, expected 3, have 2}} float j1 = (p2->x); // expected-error {{too few arguments to function call, expected 2, have 0}} ((p2->x)[23])[1][2] = *argv; // expected-error {{too many arguments to function call, expected 3, have 4}} + argv = p2->x[11][22] = argc; // expected-error {{assigning to 'char **' from incompatible type 'float'}} return ++(((p2->x)[23])); // expected-error {{too few arguments to function call, expected 2, have 1}} } Index: test/CodeGenCXX/ms-property.cpp === --- test/CodeGenCXX/ms-property.cpp +++ test/CodeGenCXX/ms-property.cpp @@ -31,7 +31,7 @@ __declspec(property(get=GetX,put=PutX)) T x[]; T GetX(T i, T j) { return i+j; } T GetX() { return 0; } - void PutX(T i, T j, T k) { j = i = k; } + T PutX(T i, T j, T k) { return j = i = k; } __declspec(property(get=GetY,put=PutY)) T y[]; char GetY(char i, Test1 j) { return i+j.get_x(); } void PutY(char i, int j, double k) { j = i = k; } @@ -61,14 +61,16 @@ // CHECK: call float @"\01?GetX@?$St@M@@QEAAMMM@Z"(%class.St* %{{.+}}, float 2.23e+02, float 1.10e+01) float j1 = p2->x[223][11]; // CHECK: [[J1:%.+]] = load float, float* % - // CHECK-NEXT: call void @"\01?PutX@?$St@M@@QEAAXMMM@Z
Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.
rjmccall added a comment. Hmm. I think a better approach would be for buildAssignmentOperation to do this; but before we figure out how to do that, we should make sure of the language semantics we're implementing. Are the semantics that the result of an assignment is always the result of the Put operation, or, if that's void, should the result be the assigned value? And what should happen with pre-increment and post-increment? http://reviews.llvm.org/D15174 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.
John, the result is always the result of Put operation. For pre-increment we have to return new value, for the post-increment - previous value returned by Get (checked it on MSVC). So, currently postincrement works correctly, pre-increment and assignment not. For pre-increment and assignment we have to capture the result of Put operation as a result. It means, that in your solution we need to modify buildAssignmentOperation() and buildIncDecOperation() and the worst thing is that these functions must be changed only for MSPropertySubscriptExpr. Best regards, Alexey Bataev = Software Engineer Intel Compiler Team 03.12.2015 20:18, John McCall пишет: > rjmccall added a comment. > > Hmm. I think a better approach would be for buildAssignmentOperation to do > this; but before we figure out how to do that, we should make sure of the > language semantics we're implementing. Are the semantics that the result of > an assignment is always the result of the Put operation, or, if that's void, > should the result be the assigned value? And what should happen with > pre-increment and post-increment? > > > http://reviews.llvm.org/D15174 > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.
rjmccall added a comment. In http://reviews.llvm.org/D15174#302248, @ABataev wrote: > John, > the result is always the result of Put operation. For pre-increment we > have to return new value, for the post-increment - previous value > returned by Get (checked it on MSVC). > So, currently postincrement works correctly, pre-increment and > assignment not. For pre-increment and assignment we have to capture the > result of Put operation as a result. > It means, that in your solution we need to modify > > buildAssignmentOperation() and buildIncDecOperation() and the worst thing is > that these functions must be changed only for MSPropertySubscriptExpr. Just property subscripts and not MS property references in general? Fortunately, PseudoOpBuilder is already a dynamic class with expression-specific subclasses, so hooking these processes is quite easy; you should be able to just add a virtual method which tells them whether the result of these compound operations should be the value that's passed to the setter or the formal result of the setter. Something like this: virtual bool useSetterResultAsExprResult() const { return false; } If that returns false, they should try to capture the set value as the result; otherwise, they should try to capture the setter result. http://reviews.llvm.org/D15174 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.
John, Actually both, subscripts and references. Ok, got will do. Best regards, Alexey Bataev = Software Engineer Intel Compiler Team 04.12.2015 12:00, John McCall пишет: > rjmccall added a comment. > > In http://reviews.llvm.org/D15174#302248, @ABataev wrote: > >> John, >> the result is always the result of Put operation. For pre-increment we >> have to return new value, for the post-increment - previous value >> returned by Get (checked it on MSVC). >> So, currently postincrement works correctly, pre-increment and >> assignment not. For pre-increment and assignment we have to capture the >> result of Put operation as a result. >> It means, that in your solution we need to modify >> >> buildAssignmentOperation() and buildIncDecOperation() and the worst thing is >> that these functions must be changed only for MSPropertySubscriptExpr. > > Just property subscripts and not MS property references in general? > > Fortunately, PseudoOpBuilder is already a dynamic class with > expression-specific subclasses, so hooking these processes is quite easy; you > should be able to just add a virtual method which tells them whether the > result of these compound operations should be the value that's passed to the > setter or the formal result of the setter. Something like this: > >virtual bool useSetterResultAsExprResult() const { return false; } > > If that returns false, they should try to capture the set value as the > result; otherwise, they should try to capture the setter result. > > > http://reviews.llvm.org/D15174 > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.
ABataev updated this revision to Diff 41848. ABataev added a comment. Update after review http://reviews.llvm.org/D15174 Files: lib/Sema/SemaPseudoObject.cpp test/CodeGenCXX/ms-property.cpp test/SemaCXX/ms-property-error.cpp test/SemaCXX/ms-property.cpp Index: lib/Sema/SemaPseudoObject.cpp === --- lib/Sema/SemaPseudoObject.cpp +++ lib/Sema/SemaPseudoObject.cpp @@ -229,7 +229,7 @@ } /// Return true if assignments have a non-void result. -bool CanCaptureValue(Expr *exp) { +bool CanCaptureValue(Expr *exp) const { if (exp->isGLValue()) return true; QualType ty = exp->getType(); @@ -245,6 +245,7 @@ virtual ExprResult buildGet() = 0; virtual ExprResult buildSet(Expr *, SourceLocation, bool captureSetValueAsResult) = 0; +virtual bool useSetterResultAsExprResult(Expr *) const { return false; } }; /// A PseudoOpBuilder for Objective-C \@properties. @@ -339,6 +340,7 @@ Expr *rebuildAndCaptureObject(Expr *) override; ExprResult buildGet() override; ExprResult buildSet(Expr *op, SourceLocation, bool) override; + bool useSetterResultAsExprResult(Expr *) const override; }; } @@ -458,6 +460,8 @@ result = buildSet(result.get(), opcLoc, /*captureSetValueAsResult*/ true); if (result.isInvalid()) return ExprError(); addSemanticExpr(result.get()); + if (useSetterResultAsExprResult(result.get())) +setResultToLastSemantic(); return complete(syntactic); } @@ -502,6 +506,9 @@ result = buildSet(result.get(), opcLoc, UnaryOperator::isPrefix(opcode)); if (result.isInvalid()) return ExprError(); addSemanticExpr(result.get()); + if (UnaryOperator::isPrefix(opcode) && + useSetterResultAsExprResult(result.get())) +setResultToLastSemantic(); UnaryOperator *syntactic = new (S.Context) UnaryOperator(syntacticOp, opcode, resultType, @@ -1497,6 +1504,12 @@ op->getSourceRange().getEnd()); } +bool MSPropertyOpBuilder::useSetterResultAsExprResult(Expr *ResExpr) const { + assert(ResExpr); + return !ResExpr->getType()->isVoidType() && + (ResExpr->getType()->isDependentType() || CanCaptureValue(ResExpr)); +} + //===--===// // General Sema routines. //===--===// Index: test/SemaCXX/ms-property.cpp === --- test/SemaCXX/ms-property.cpp +++ test/SemaCXX/ms-property.cpp @@ -29,7 +29,7 @@ public: __declspec(property(get=GetX,put=PutX)) T x[]; T GetX(T i, T j) { return i+j; } - void PutX(T i, T j, T k) { j = i = k; } + T PutX(T i, T j, T k) { return j = i = k; } ~St() { x[0][0] = x[1][1]; } }; @@ -52,6 +52,8 @@ ((p2->x)[23])[1] = j1; // CHECK-NEXT: ++(((p2->x)[23])[1]); ++(((p2->x)[23])[1]); + // CHECK-NEXT: j1 = ((p2->x)[23])[1] = j1; + j1 = ((p2->x)[23])[1] = j1; // CHECK-NEXT: return Test1::GetTest1()->X; return Test1::GetTest1()->X; } Index: test/SemaCXX/ms-property-error.cpp === --- test/SemaCXX/ms-property-error.cpp +++ test/SemaCXX/ms-property-error.cpp @@ -7,17 +7,19 @@ void PutX(int i, int j, int k) { j = i = k; } // expected-note {{'PutX' declared here}} }; +char *ptr; template class St { public: __declspec(property(get=GetX,put=PutX)) T x[]; T GetX(T i, T j) { return i+j; } // expected-note 3 {{'GetX' declared here}} - void PutX(T i, T j, T k) { j = i = k; } // expected-note 2 {{'PutX' declared here}} + T PutX(T i, T j, T k) { return j = i = k; } // expected-note 2 {{'PutX' declared here}} ~St() { x[1] = 0; // expected-error {{too few arguments to function call, expected 3, have 2}} x[2][3] = 4; ++x[2][3]; x[1][2] = x[3][4][5]; // expected-error {{too many arguments to function call, expected 2, have 3}} +ptr = x[1][2] = x[3][4]; // expected-error {{assigning to 'char *' from incompatible type 'int'}} } }; @@ -30,5 +32,6 @@ (p1->x[23]) = argc; // expected-error {{too few arguments to function call, expected 3, have 2}} float j1 = (p2->x); // expected-error {{too few arguments to function call, expected 2, have 0}} ((p2->x)[23])[1][2] = *argv; // expected-error {{too many arguments to function call, expected 3, have 4}} + argv = p2->x[11][22] = argc; // expected-error {{assigning to 'char **' from incompatible type 'float'}} return ++(((p2->x)[23])); // expected-error {{too few arguments to function call, expected 2, have 1}} } Index: test/CodeGenCXX/ms-property.cpp === --- test/CodeGenCXX/ms-property.cpp +++ test/CodeGenCXX/ms-property.cpp @@ -31,7 +31,7 @@ __declspec(property(get=GetX,put=PutX)) T x[]; T GetX(T i, T j) { return i+j; }
Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.
rjmccall added inline comments. Comment at: lib/Sema/SemaPseudoObject.cpp:232 @@ -231,3 +231,3 @@ /// Return true if assignments have a non-void result. -bool CanCaptureValue(Expr *exp) { +bool CanCaptureValue(Expr *exp) const { if (exp->isGLValue()) Just make this static. Comment at: lib/Sema/SemaPseudoObject.cpp:464 @@ -461,1 +463,3 @@ + if (useSetterResultAsExprResult(result.get())) +setResultToLastSemantic(); This will leave the result as the captured set value if it can't capture the setter result. Is that desired? http://reviews.llvm.org/D15174 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.
ABataev marked 2 inline comments as done. Comment at: lib/Sema/SemaPseudoObject.cpp:232 @@ -231,3 +231,3 @@ /// Return true if assignments have a non-void result. -bool CanCaptureValue(Expr *exp) { +bool CanCaptureValue(Expr *exp) const { if (exp->isGLValue()) rjmccall wrote: > Just make this static. Ok Comment at: lib/Sema/SemaPseudoObject.cpp:464 @@ -461,1 +463,3 @@ + if (useSetterResultAsExprResult(result.get())) +setResultToLastSemantic(); rjmccall wrote: > This will leave the result as the captured set value if it can't capture the > setter result. Is that desired? The fact that the value can be captured must be checked in useSetterResultAsExprResult() http://reviews.llvm.org/D15174 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.
ABataev updated this revision to Diff 42026. ABataev marked 2 inline comments as done. ABataev added a comment. Update after review http://reviews.llvm.org/D15174 Files: lib/Sema/SemaPseudoObject.cpp test/CodeGenCXX/ms-property.cpp test/SemaCXX/ms-property-error.cpp test/SemaCXX/ms-property.cpp Index: lib/Sema/SemaPseudoObject.cpp === --- lib/Sema/SemaPseudoObject.cpp +++ lib/Sema/SemaPseudoObject.cpp @@ -229,7 +229,7 @@ } /// Return true if assignments have a non-void result. -bool CanCaptureValue(Expr *exp) { +static bool CanCaptureValue(Expr *exp) { if (exp->isGLValue()) return true; QualType ty = exp->getType(); @@ -245,6 +245,7 @@ virtual ExprResult buildGet() = 0; virtual ExprResult buildSet(Expr *, SourceLocation, bool captureSetValueAsResult) = 0; +virtual bool useSetterResultAsExprResult(Expr *) const { return false; } }; /// A PseudoOpBuilder for Objective-C \@properties. @@ -339,6 +340,7 @@ Expr *rebuildAndCaptureObject(Expr *) override; ExprResult buildGet() override; ExprResult buildSet(Expr *op, SourceLocation, bool) override; + bool useSetterResultAsExprResult(Expr *) const override; }; } @@ -458,6 +460,8 @@ result = buildSet(result.get(), opcLoc, /*captureSetValueAsResult*/ true); if (result.isInvalid()) return ExprError(); addSemanticExpr(result.get()); + if (useSetterResultAsExprResult(result.get())) +setResultToLastSemantic(); return complete(syntactic); } @@ -502,6 +506,9 @@ result = buildSet(result.get(), opcLoc, UnaryOperator::isPrefix(opcode)); if (result.isInvalid()) return ExprError(); addSemanticExpr(result.get()); + if (UnaryOperator::isPrefix(opcode) && + useSetterResultAsExprResult(result.get())) +setResultToLastSemantic(); UnaryOperator *syntactic = new (S.Context) UnaryOperator(syntacticOp, opcode, resultType, @@ -1497,6 +1504,12 @@ op->getSourceRange().getEnd()); } +bool MSPropertyOpBuilder::useSetterResultAsExprResult(Expr *ResExpr) const { + assert(ResExpr); + return !ResExpr->getType()->isVoidType() && + (ResExpr->getType()->isDependentType() || CanCaptureValue(ResExpr)); +} + //===--===// // General Sema routines. //===--===// Index: test/SemaCXX/ms-property.cpp === --- test/SemaCXX/ms-property.cpp +++ test/SemaCXX/ms-property.cpp @@ -29,7 +29,7 @@ public: __declspec(property(get=GetX,put=PutX)) T x[]; T GetX(T i, T j) { return i+j; } - void PutX(T i, T j, T k) { j = i = k; } + T PutX(T i, T j, T k) { return j = i = k; } ~St() { x[0][0] = x[1][1]; } }; @@ -52,6 +52,8 @@ ((p2->x)[23])[1] = j1; // CHECK-NEXT: ++(((p2->x)[23])[1]); ++(((p2->x)[23])[1]); + // CHECK-NEXT: j1 = ((p2->x)[23])[1] = j1; + j1 = ((p2->x)[23])[1] = j1; // CHECK-NEXT: return Test1::GetTest1()->X; return Test1::GetTest1()->X; } Index: test/SemaCXX/ms-property-error.cpp === --- test/SemaCXX/ms-property-error.cpp +++ test/SemaCXX/ms-property-error.cpp @@ -7,17 +7,19 @@ void PutX(int i, int j, int k) { j = i = k; } // expected-note {{'PutX' declared here}} }; +char *ptr; template class St { public: __declspec(property(get=GetX,put=PutX)) T x[]; T GetX(T i, T j) { return i+j; } // expected-note 3 {{'GetX' declared here}} - void PutX(T i, T j, T k) { j = i = k; } // expected-note 2 {{'PutX' declared here}} + T PutX(T i, T j, T k) { return j = i = k; } // expected-note 2 {{'PutX' declared here}} ~St() { x[1] = 0; // expected-error {{too few arguments to function call, expected 3, have 2}} x[2][3] = 4; ++x[2][3]; x[1][2] = x[3][4][5]; // expected-error {{too many arguments to function call, expected 2, have 3}} +ptr = x[1][2] = x[3][4]; // expected-error {{assigning to 'char *' from incompatible type 'int'}} } }; @@ -30,5 +32,6 @@ (p1->x[23]) = argc; // expected-error {{too few arguments to function call, expected 3, have 2}} float j1 = (p2->x); // expected-error {{too few arguments to function call, expected 2, have 0}} ((p2->x)[23])[1][2] = *argv; // expected-error {{too many arguments to function call, expected 3, have 4}} + argv = p2->x[11][22] = argc; // expected-error {{assigning to 'char **' from incompatible type 'float'}} return ++(((p2->x)[23])); // expected-error {{too few arguments to function call, expected 2, have 1}} } Index: test/CodeGenCXX/ms-property.cpp === --- test/CodeGenCXX/ms-property.cpp +++ test/CodeGenCXX/ms-property.cpp @@ -31,7 +31,7 @@ __declspec(property(get=GetX,put=PutX)) T
Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.
rjmccall added inline comments. Comment at: lib/Sema/SemaPseudoObject.cpp:464 @@ -461,1 +463,3 @@ + if (useSetterResultAsExprResult(result.get())) +setResultToLastSemantic(); Sure, but that's not what I'm saying. I'm saying that the code, as you've written it, will use the value passed to the setter (if capturable) as the result of the expression if it can't capture the setter result. That is, given: struct A { __declspec(property(get=GetX,put=PutX)) int x; int GetX() const { return 0; } void SetX(long y) {} }; this expression will have type long instead of void, because it's implicitly falling back on capturing the value passed to the setter: a.x = 5; I suspect that that's not the MSVC semantics, and the language rule is that you should always use the setter result. If that's true, you should be asking the subclass which rule to use abstractly, i.e. before constructing the setter, and then (1) suppressing the capture of the set value and (2) using the setter result (if capturable) or else nothing. http://reviews.llvm.org/D15174 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.
John, Your example won't be compiled at all. Clang and MSVC emit error messages in this case. The next code struct A { __declspec(property(get=GetX,put=SetX)) int x; int GetX() const { return 0; } void SetX(long y) {} }; a.x = 5; compiled fine by MSVC and clang and provides correct result on clang (i.e. the result value can't be captured at all, because SetX() returns void) return a.x = 5; clang: error: cannot initialize return object of type 'int' with an rvalue of type 'void' ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.
rjmccall added a comment. I don't understand why that's true. buildSet is called with captureSetValueAsResult=true, and the set value is definitely capturable, so that value should be set as the result; and you're not overriding it. Why does the expression end up having type void? http://reviews.llvm.org/D15174 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.
Set value is a call to SetX() function, neither the argument of the SetX(), nor the result of the GetX(). So, expression a.x=5 always emits the following code as a result a.Setx(5). MSPropertyRefBuilder::buildSet() does not capture results at all. It just ignores captureSetValueAsResult value. That's why we have to capture final result after 'set' is built completely. Best regards, Alexey Bataev = Software Engineer Intel Compiler Team 07.12.2015 11:07, John McCall пишет: > rjmccall added a comment. > > I don't understand why that's true. buildSet is called with > captureSetValueAsResult=true, and the set value is definitely capturable, so > that value should be set as the result; and you're not overriding it. Why > does the expression end up having type void? > > > http://reviews.llvm.org/D15174 > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.
rjmccall added a comment. Oh, I see, of course. It would be clearer if you asked the subclass which value to use abstractly (i.e. independently of the actual set expression) and then passed down captureSetValueAsResult=false when the subclass says to use the setter result. http://reviews.llvm.org/D15174 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.
Ok, I will implement this logic Best regards, Alexey Bataev = Software Engineer Intel Compiler Team 07.12.2015 22:30, John McCall пишет: > rjmccall added a comment. > > Oh, I see, of course. > > It would be clearer if you asked the subclass which value to use abstractly > (i.e. independently of the actual set expression) and then passed down > captureSetValueAsResult=false when the subclass says to use the setter result. > > > http://reviews.llvm.org/D15174 > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.
ABataev updated this revision to Diff 42140. ABataev added a comment. Update after review http://reviews.llvm.org/D15174 Files: lib/Sema/SemaPseudoObject.cpp test/CodeGenCXX/ms-property.cpp test/SemaCXX/ms-property-error.cpp test/SemaCXX/ms-property.cpp Index: lib/Sema/SemaPseudoObject.cpp === --- lib/Sema/SemaPseudoObject.cpp +++ lib/Sema/SemaPseudoObject.cpp @@ -229,7 +229,7 @@ } /// Return true if assignments have a non-void result. -bool CanCaptureValue(Expr *exp) { +static bool CanCaptureValue(Expr *exp) { if (exp->isGLValue()) return true; QualType ty = exp->getType(); @@ -245,6 +245,8 @@ virtual ExprResult buildGet() = 0; virtual ExprResult buildSet(Expr *, SourceLocation, bool captureSetValueAsResult) = 0; +virtual bool useSetterResultAsExprResult(Expr *) const { return false; } +virtual bool captureSetValueAsResult() const { return true; } }; /// A PseudoOpBuilder for Objective-C \@properties. @@ -339,6 +341,8 @@ Expr *rebuildAndCaptureObject(Expr *) override; ExprResult buildGet() override; ExprResult buildSet(Expr *op, SourceLocation, bool) override; + bool useSetterResultAsExprResult(Expr *) const override; + bool captureSetValueAsResult() const override { return false; } }; } @@ -455,9 +459,11 @@ // The result of the assignment, if not void, is the value set into // the l-value. - result = buildSet(result.get(), opcLoc, /*captureSetValueAsResult*/ true); + result = buildSet(result.get(), opcLoc, captureSetValueAsResult()); if (result.isInvalid()) return ExprError(); addSemanticExpr(result.get()); + if (useSetterResultAsExprResult(result.get())) +setResultToLastSemantic(); return complete(syntactic); } @@ -499,9 +505,13 @@ // Store that back into the result. The value stored is the result // of a prefix operation. - result = buildSet(result.get(), opcLoc, UnaryOperator::isPrefix(opcode)); + result = buildSet(result.get(), opcLoc, UnaryOperator::isPrefix(opcode) && + captureSetValueAsResult()); if (result.isInvalid()) return ExprError(); addSemanticExpr(result.get()); + if (UnaryOperator::isPrefix(opcode) && + useSetterResultAsExprResult(result.get())) +setResultToLastSemantic(); UnaryOperator *syntactic = new (S.Context) UnaryOperator(syntacticOp, opcode, resultType, @@ -1497,6 +1507,12 @@ op->getSourceRange().getEnd()); } +bool MSPropertyOpBuilder::useSetterResultAsExprResult(Expr *ResExpr) const { + assert(ResExpr); + return !ResExpr->getType()->isVoidType() && + (ResExpr->getType()->isDependentType() || CanCaptureValue(ResExpr)); +} + //===--===// // General Sema routines. //===--===// Index: test/SemaCXX/ms-property.cpp === --- test/SemaCXX/ms-property.cpp +++ test/SemaCXX/ms-property.cpp @@ -29,7 +29,7 @@ public: __declspec(property(get=GetX,put=PutX)) T x[]; T GetX(T i, T j) { return i+j; } - void PutX(T i, T j, T k) { j = i = k; } + T PutX(T i, T j, T k) { return j = i = k; } ~St() { x[0][0] = x[1][1]; } }; @@ -52,6 +52,8 @@ ((p2->x)[23])[1] = j1; // CHECK-NEXT: ++(((p2->x)[23])[1]); ++(((p2->x)[23])[1]); + // CHECK-NEXT: j1 = ((p2->x)[23])[1] = j1; + j1 = ((p2->x)[23])[1] = j1; // CHECK-NEXT: return Test1::GetTest1()->X; return Test1::GetTest1()->X; } Index: test/SemaCXX/ms-property-error.cpp === --- test/SemaCXX/ms-property-error.cpp +++ test/SemaCXX/ms-property-error.cpp @@ -7,17 +7,19 @@ void PutX(int i, int j, int k) { j = i = k; } // expected-note {{'PutX' declared here}} }; +char *ptr; template class St { public: __declspec(property(get=GetX,put=PutX)) T x[]; T GetX(T i, T j) { return i+j; } // expected-note 3 {{'GetX' declared here}} - void PutX(T i, T j, T k) { j = i = k; } // expected-note 2 {{'PutX' declared here}} + T PutX(T i, T j, T k) { return j = i = k; } // expected-note 2 {{'PutX' declared here}} ~St() { x[1] = 0; // expected-error {{too few arguments to function call, expected 3, have 2}} x[2][3] = 4; ++x[2][3]; x[1][2] = x[3][4][5]; // expected-error {{too many arguments to function call, expected 2, have 3}} +ptr = x[1][2] = x[3][4]; // expected-error {{assigning to 'char *' from incompatible type 'int'}} } }; @@ -30,5 +32,6 @@ (p1->x[23]) = argc; // expected-error {{too few arguments to function call, expected 3, have 2}} float j1 = (p2->x); // expected-error {{too few arguments to function call, expected 2, have 0}} ((p2->x)[23])[1][2] = *argv; // expected-error {{too
Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.
rjmccall added a comment. Thanks! Comment at: lib/Sema/SemaPseudoObject.cpp:249 @@ -248,1 +248,3 @@ +virtual bool useSetterResultAsExprResult(Expr *) const { return false; } +virtual bool captureSetValueAsResult() const { return true; } }; I think you just need one of these. If useSetterResultAsExprResult() returns true, buildAssignmentOperation and buildIncDecOperation should try to capture the setter result; otherwise, they should try to capture the set value. And it doesn't need to take an Expr* anymore. Please add a comment explaining that; something like this: /// Should the result of an assignment be the formal result of the setter /// call or the value that was passed to the setter? /// /// Different pseudo-object language features use different language rules for this. /// The default is to use the set value. Currently, this affects the behavior of simple /// assignments, compound assignments, and prefix increment and decrement. /// Postfix increment and decrement always use the getter result as the expression /// result. /// /// If this method returns false, and the set value isn't capturable for some /// reason, the result of the expression will be void. http://reviews.llvm.org/D15174 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.
ABataev updated this revision to Diff 42282. ABataev added a comment. Update after review http://reviews.llvm.org/D15174 Files: lib/Sema/SemaPseudoObject.cpp test/CodeGenCXX/ms-property.cpp test/SemaCXX/ms-property-error.cpp test/SemaCXX/ms-property.cpp Index: lib/Sema/SemaPseudoObject.cpp === --- lib/Sema/SemaPseudoObject.cpp +++ lib/Sema/SemaPseudoObject.cpp @@ -229,7 +229,7 @@ } /// Return true if assignments have a non-void result. -bool CanCaptureValue(Expr *exp) { +static bool CanCaptureValue(Expr *exp) { if (exp->isGLValue()) return true; QualType ty = exp->getType(); @@ -245,6 +245,20 @@ virtual ExprResult buildGet() = 0; virtual ExprResult buildSet(Expr *, SourceLocation, bool captureSetValueAsResult) = 0; +/// \brief Should the result of an assignment be the formal result of the +/// setter call or the value that was passed to the setter? +/// +/// Different pseudo-object language features use different language rules +/// for this. +/// The default is to use the set value. Currently, this affects the +/// behavior of simple assignments, compound assignments, and prefix +/// increment and decrement. +/// Postfix increment and decrement always use the getter result as the +/// expression result. +/// +/// If this method returns false, and the set value isn't capturable for +/// some reason, the result of the expression will be void. +virtual bool captureSetValueAsResult() const { return true; } }; /// A PseudoOpBuilder for Objective-C \@properties. @@ -339,6 +353,7 @@ Expr *rebuildAndCaptureObject(Expr *) override; ExprResult buildGet() override; ExprResult buildSet(Expr *op, SourceLocation, bool) override; + bool captureSetValueAsResult() const override { return false; } }; } @@ -455,9 +470,12 @@ // The result of the assignment, if not void, is the value set into // the l-value. - result = buildSet(result.get(), opcLoc, /*captureSetValueAsResult*/ true); + result = buildSet(result.get(), opcLoc, captureSetValueAsResult()); if (result.isInvalid()) return ExprError(); addSemanticExpr(result.get()); + if (!captureSetValueAsResult() && !result.get()->getType()->isVoidType() && + (result.get()->isTypeDependent() || CanCaptureValue(result.get( +setResultToLastSemantic(); return complete(syntactic); } @@ -499,9 +517,14 @@ // Store that back into the result. The value stored is the result // of a prefix operation. - result = buildSet(result.get(), opcLoc, UnaryOperator::isPrefix(opcode)); + result = buildSet(result.get(), opcLoc, UnaryOperator::isPrefix(opcode) && + captureSetValueAsResult()); if (result.isInvalid()) return ExprError(); addSemanticExpr(result.get()); + if (UnaryOperator::isPrefix(opcode) && !captureSetValueAsResult() && + !result.get()->getType()->isVoidType() && + (result.get()->isTypeDependent() || CanCaptureValue(result.get( +setResultToLastSemantic(); UnaryOperator *syntactic = new (S.Context) UnaryOperator(syntacticOp, opcode, resultType, Index: test/SemaCXX/ms-property.cpp === --- test/SemaCXX/ms-property.cpp +++ test/SemaCXX/ms-property.cpp @@ -29,7 +29,7 @@ public: __declspec(property(get=GetX,put=PutX)) T x[]; T GetX(T i, T j) { return i+j; } - void PutX(T i, T j, T k) { j = i = k; } + T PutX(T i, T j, T k) { return j = i = k; } ~St() { x[0][0] = x[1][1]; } }; @@ -52,6 +52,8 @@ ((p2->x)[23])[1] = j1; // CHECK-NEXT: ++(((p2->x)[23])[1]); ++(((p2->x)[23])[1]); + // CHECK-NEXT: j1 = ((p2->x)[23])[1] = j1; + j1 = ((p2->x)[23])[1] = j1; // CHECK-NEXT: return Test1::GetTest1()->X; return Test1::GetTest1()->X; } Index: test/SemaCXX/ms-property-error.cpp === --- test/SemaCXX/ms-property-error.cpp +++ test/SemaCXX/ms-property-error.cpp @@ -7,17 +7,19 @@ void PutX(int i, int j, int k) { j = i = k; } // expected-note {{'PutX' declared here}} }; +char *ptr; template class St { public: __declspec(property(get=GetX,put=PutX)) T x[]; T GetX(T i, T j) { return i+j; } // expected-note 3 {{'GetX' declared here}} - void PutX(T i, T j, T k) { j = i = k; } // expected-note 2 {{'PutX' declared here}} + T PutX(T i, T j, T k) { return j = i = k; } // expected-note 2 {{'PutX' declared here}} ~St() { x[1] = 0; // expected-error {{too few arguments to function call, expected 3, have 2}} x[2][3] = 4; ++x[2][3]; x[1][2] = x[3][4][5]; // expected-error {{too many arguments to function call, expected 2, have 3}} +ptr = x[1][2] = x[3][4]; // expected-error {{assigning to 'char *' from incompatible type 'int'}} } }; @@ -30,5 +32,6 @@
Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.
rjmccall added a comment. Thanks! Just a slight tweak to the comment, then LGTM. Comment at: lib/Sema/SemaPseudoObject.cpp:259 @@ +258,3 @@ +/// +/// If this method returns false, and the set value isn't capturable for +/// some reason, the result of the expression will be void. "If this method returns *true*", since you picked this one. http://reviews.llvm.org/D15174 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.
ABataev marked an inline comment as done. Comment at: lib/Sema/SemaPseudoObject.cpp:259 @@ +258,3 @@ +/// +/// If this method returns false, and the set value isn't capturable for +/// some reason, the result of the expression will be void. rjmccall wrote: > "If this method returns *true*", since you picked this one. Oops, thanks http://reviews.llvm.org/D15174 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.
This revision was automatically updated to reflect the committed changes. ABataev marked an inline comment as done. Closed by commit rL255218: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not… (authored by ABataev). Changed prior to commit: http://reviews.llvm.org/D15174?vs=42282&id=42373#toc Repository: rL LLVM http://reviews.llvm.org/D15174 Files: cfe/trunk/lib/Sema/SemaPseudoObject.cpp cfe/trunk/test/CodeGenCXX/ms-property.cpp cfe/trunk/test/SemaCXX/ms-property-error.cpp cfe/trunk/test/SemaCXX/ms-property.cpp Index: cfe/trunk/test/CodeGenCXX/ms-property.cpp === --- cfe/trunk/test/CodeGenCXX/ms-property.cpp +++ cfe/trunk/test/CodeGenCXX/ms-property.cpp @@ -31,7 +31,7 @@ __declspec(property(get=GetX,put=PutX)) T x[]; T GetX(T i, T j) { return i+j; } T GetX() { return 0; } - void PutX(T i, T j, T k) { j = i = k; } + T PutX(T i, T j, T k) { return j = i = k; } __declspec(property(get=GetY,put=PutY)) T y[]; char GetY(char i, Test1 j) { return i+j.get_x(); } void PutY(char i, int j, double k) { j = i = k; } @@ -61,14 +61,16 @@ // CHECK: call float @"\01?GetX@?$St@M@@QEAAMMM@Z"(%class.St* %{{.+}}, float 2.23e+02, float 1.10e+01) float j1 = p2->x[223][11]; // CHECK: [[J1:%.+]] = load float, float* % - // CHECK-NEXT: call void @"\01?PutX@?$St@M@@QEAAXMMM@Z"(%class.St* %{{.+}}, float 2.30e+01, float 1.00e+00, float [[J1]]) - p2->x[23][1] = j1; + // CHECK-NEXT: [[CALL:%.+]] = call float @"\01?PutX@?$St@M@@QEAA@Z"(%class.St* %{{.+}}, float 2.30e+01, float 1.00e+00, float [[J1]]) + // CHECK-NEXT: [[CONV:%.+]] = fptosi float [[CALL]] to i32 + // CHECK-NEXT: store i32 [[CONV]], i32* + argc = p2->x[23][1] = j1; // CHECK: [[IDX:%.+]] = call i32 @"\01?idx@@YAHXZ"() // CHECK-NEXT: [[CONV:%.+]] = sitofp i32 [[IDX]] to float // CHECK-NEXT: [[GET:%.+]] = call float @"\01?GetX@?$St@M@@QEAAMMM@Z"(%class.St* %{{.+}}, float [[CONV]], float 1.00e+00) // CHECK-NEXT: [[INC:%.+]] = fadd float [[GET]], 1.00e+00 // CHECK-NEXT: [[CONV:%.+]] = sitofp i32 [[IDX]] to float - // CHECK-NEXT: call void @"\01?PutX@?$St@M@@QEAAXMMM@Z"(%class.St* %{{.+}}, float [[CONV]], float 1.00e+00, float [[INC]]) + // CHECK-NEXT: call float @"\01?PutX@?$St@M@@QEAA@Z"(%class.St* %{{.+}}, float [[CONV]], float 1.00e+00, float [[INC]]) ++p2->x[idx()][1]; // CHECK: call void @"\01??$foo@H@@YAXHH@Z"(i32 %{{.+}}, i32 %{{.+}}) foo(argc, (int)argv[0][0]); @@ -93,19 +95,19 @@ // CHECK: [[CAST1:%.+]] = sitofp i32 [[J]] to float // CHECK: [[J:%.+]] = load i32, i32* % // CHECK: [[CAST2:%.+]] = sitofp i32 [[J]] to float - // CHECK: call void @"\01?PutX@?$St@M@@QEAAXMMM@Z"(%class.St* [[P2_1]], float [[CAST2]], float [[CAST1]], float [[CAST]]) + // CHECK: call float @"\01?PutX@?$St@M@@QEAA@Z"(%class.St* [[P2_1]], float [[CAST2]], float [[CAST1]], float [[CAST]]) p2->x[j][j] = p2->y[p1->x[argc][0]][t]; // CHECK: [[CALL:%.+]] = call %class.Test1* @"\01?GetTest1@Test1@@SAPEAV1@XZ"() // CHECK-NEXT: call i32 @"\01?get_x@Test1@@QEBAHXZ"(%class.Test1* [[CALL]]) return Test1::GetTest1()->X; } // CHECK: define linkonce_odr void @"\01??$foo@H@@YAXHH@Z"(i32 %{{.+}}, i32 %{{.+}}) // CHECK: call i32 @"\01?GetX@?$St@H@@QEAAHHH@Z"(%class.St{{.+}}* [[BAR:%.+]], i32 %{{.+}} i32 %{{.+}}) -// CHECK: call void @"\01?PutX@?$St@H@@QEAAXHHH@Z"(%class.St{{.+}}* [[BAR]], i32 %{{.+}}, i32 %{{.+}}, i32 %{{.+}}) +// CHECK: call i32 @"\01?PutX@?$St@H@@QEAA@Z"(%class.St{{.+}}* [[BAR]], i32 %{{.+}}, i32 %{{.+}}, i32 %{{.+}}) // CHECK: call i32 @"\01?GetX@?$St@H@@QEAAHHH@Z"(%class.St{{.+}}* [[BAR]], i32 %{{.+}} i32 %{{.+}}) // CHECK: call void @"\01?PutY@?$St@H@@QEAAXDHN@Z"(%class.St{{.+}}* [[BAR]], i8 %{{.+}}, i32 %{{.+}}, double %{{.+}} // CHECK: call i32 @"\01?GetX@?$St@H@@QEAAHHH@Z"(%class.St{{.+}}* [[BAR]], i32 %{{.+}} i32 %{{.+}}) // CHECK: call i8 @"\01?GetY@?$St@H@@QEAADDVTest1@@@Z"(%class.St{{.+}}* [[BAR]], i8 %{{.+}}, %class.Test1* %{{.+}}) -// CHECK: call void @"\01?PutX@?$St@H@@QEAAXHHH@Z"(%class.St{{.+}}* [[BAR]], i32 %{{.+}}, i32 %{{.+}}, i32 %{{.+}}) +// CHECK: call i32 @"\01?PutX@?$St@H@@QEAA@Z"(%class.St{{.+}}* [[BAR]], i32 %{{.+}}, i32 %{{.+}}, i32 %{{.+}}) #endif //HEADER Index: cfe/trunk/test/SemaCXX/ms-property-error.cpp === --- cfe/trunk/test/SemaCXX/ms-property-error.cpp +++ cfe/trunk/test/SemaCXX/ms-property-error.cpp @@ -7,17 +7,19 @@ void PutX(int i, int j, int k) { j = i = k; } // expected-note {{'PutX' declared here}} }; +char *ptr; template class St { public: __declspec(property(get=GetX,put=PutX)) T x[]; T GetX(T i, T j) { return i+j; } // expected-note 3 {{'GetX' declared here}} - void PutX(T i, T j, T k) { j = i = k; } // expected-note 2 {{'PutX' declared here}} + T PutX(T i, T j, T k) { return j = i = k; } // expected-note 2 {{'PutX'