This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ABataev on Dec 2 2015, 8:06 PM.

Details

Summary

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

Diff Detail

Event Timeline

ABataev updated this revision to Diff 41711.Dec 2 2015, 8:06 PM
ABataev retitled this revision from to [MSVC] Fix for http://llvm.org/PR25636: indexed accessor property not supported correctly..
ABataev updated this object.
ABataev added reviewers: rnk, rjmccall.
ABataev added a subscriber: cfe-commits.
rjmccall edited edge metadata.Dec 3 2015, 9:18 AM

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?

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

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.

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

ABataev updated this revision to Diff 41848.Dec 4 2015, 1:32 AM
ABataev edited edge metadata.

Update after review

rjmccall added inline comments.Dec 4 2015, 2:50 PM
lib/Sema/SemaPseudoObject.cpp
232

Just make this static.

464

This will leave the result as the captured set value if it can't capture the setter result. Is that desired?

ABataev marked 2 inline comments as done.Dec 6 2015, 8:33 PM
ABataev added inline comments.
lib/Sema/SemaPseudoObject.cpp
232

Ok

464

The fact that the value can be captured must be checked in useSetterResultAsExprResult()

ABataev updated this revision to Diff 42026.Dec 6 2015, 9:03 PM
ABataev marked 2 inline comments as done.

Update after review

rjmccall added inline comments.Dec 6 2015, 10:14 PM
lib/Sema/SemaPseudoObject.cpp
464

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.

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'

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?

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

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.

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

ABataev updated this revision to Diff 42140.Dec 7 2015, 11:02 PM

Update after review

Thanks!

lib/Sema/SemaPseudoObject.cpp
249

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.
ABataev updated this revision to Diff 42282.Dec 9 2015, 2:49 AM

Update after review

Thanks! Just a slight tweak to the comment, then LGTM.

lib/Sema/SemaPseudoObject.cpp
259

"If this method returns *true*", since you picked this one.

ABataev marked an inline comment as done.Dec 9 2015, 7:59 PM
ABataev added inline comments.
lib/Sema/SemaPseudoObject.cpp
259

Oops, thanks

This revision was automatically updated to reflect the committed changes.
ABataev marked an inline comment as done.