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
Details
Diff Detail
Event Timeline
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?
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 modifybuildAssignmentOperation() 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.
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?
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.
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. |
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. |
lib/Sema/SemaPseudoObject.cpp | ||
---|---|---|
259 | Oops, thanks |
Just make this static.