Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.

2015-12-09 Thread Alexey Bataev via cfe-commits
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:

Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.

2015-12-09 Thread Alexey Bataev via cfe-commits
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

Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.

2015-12-09 Thread Alexey Bataev via cfe-commits
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:

Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.

2015-12-09 Thread John McCall via cfe-commits
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

Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.

2015-12-08 Thread John McCall via cfe-commits
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

Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.

2015-12-07 Thread Alexey Bataev via cfe-commits
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:

Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.

2015-12-07 Thread John McCall via cfe-commits
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?

Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.

2015-12-07 Thread Alexey Bataev via cfe-commits
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

Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.

2015-12-07 Thread John McCall via cfe-commits
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.

Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.

2015-12-07 Thread Alexey Bataev via cfe-commits
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.

Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.

2015-12-06 Thread Alexey Bataev via cfe-commits
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

Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.

2015-12-06 Thread Alexey Bataev via cfe-commits
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

Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.

2015-12-06 Thread Alexey Bataev via cfe-commits
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())

Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.

2015-12-04 Thread Alexey Bataev via cfe-commits
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

Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.

2015-12-04 Thread John McCall via cfe-commits
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

Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.

2015-12-04 Thread Alexey Bataev via cfe-commits
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

Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.

2015-12-04 Thread Alexey Bataev via cfe-commits
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:

Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.

2015-12-04 Thread John McCall via cfe-commits
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())

Re: [PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.

2015-12-03 Thread John McCall via cfe-commits
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

[PATCH] D15174: [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly.

2015-12-02 Thread Alexey Bataev via cfe-commits
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