Community
Participate
Working Groups
The FVT testcases contain several tests to ensure that If expressions are evaluated left-to-right and the evaluation stops as soon as an operand is found to be false. For example, TestConditionalShortCircuitLib has this Function testIfCondition(e Event in) result boolean = false; result = false; total = 0; if(addAndReturnFalse(1) && addAndReturnFalse(2)) result = true; end but the generated code is first evaluating each of the two operands (the function calls) and then evaluating the if expression; as a result, this test fails. The root cause of this is in ReorgCode, so this is manifest in both Java and JavaScript gen.... I've attached the FVT testcase.
Created attachment 200456 [details] FVT testcase
Jeff, it looks like you wrote ReorganizeCode so I'm hoping you can look into this before your trip. I noticed that we treat Ifs differently than other statements which might contain expressions that use && or ||. For example, the two lines below don't suffer from this problem because we don't pull out their function calls. while(addAndReturnFalse(1) && addAndReturnFalse(2)) end b boolean = addAndReturnFalse(1) && addAndReturnFalse(2);
Matt, It's not true that while and boolean assignment don't have this problem. In fact they do. Take this test case: function addAndReturnFalse(a int inout) returns(boolean) return (true); end a int; while(addAndReturnFalse(a) && addAndReturnFalse(a)) end b boolean = addAndReturnFalse(a) && addAndReturnFalse(a); if (addAndReturnFalse(a) && addAndReturnFalse(a)) end Because the function being called has an inout parm, it means that the argument being passed must be replaced on function exit. If you generate that logic, you will see that unBox assignments are being done. I also think that we need to break apart these additional statements because if the 1st part of the condition was false, then the next part should not be executed. I think that while and boolean assignment must fall into the same logic as IF. I'll try to work on this, but it is complicated.
Oh, I guess I missed that (while and assignment to boolean are broken in the same way). It's actually a good thing, I think. It means you might be able deal with this situation by looking at BinaryExpressions not every place one of them might show up (If/While/etc.).
I've changed some code with this. It is better, but it still places the ezeUnbox statements for inout and out args, after the statement being processed. This means these statements would be placed after an IF, or WHILE or RETURN, etc. This is not really what is supposed to happen. This is a complex problem. I know of only 2 solutions to it: 1) we could make the usage of invoking any function that has any inout or out parms to be used in an assignment statement only. In other words, give an error if the user tries to use a function with inout or out, that is in any other type of statement. That would completely eliminate the problem. 2) either the reorg code or the IR creation code would have to break up any of these types of expressions, into a series of IF statements. This would be very complicated and prone to errors.
The first solution suggested in comment 5 is too restrictive. Also, I was right in comment 2: the function calls in an If statement's condition are broken out, but that doesn't happen in all other cases. function returnInout(parm int inout) returns (boolean) end z1, z2 int; if(returnInout(z1) && returnInout(z2)) end // Both funcs always called. while(returnInout(z1) && returnInout(z2)) end // No problem. b boolean = returnInout(z1) && returnInout(z2); // No problem.
I sort of lost track of this one; is a fix still required, and if so, shouldn't we implement it soon for such a significant fix?
Yeah, this really does need to be fixed. Please figure out if the various problems mentioned here have been fixed or not.
Paul is supposed to write a preprocessing utility that will break out binary expressions into components. All of these problems are the same and will be solved once that utility has been written.
*** This bug has been marked as a duplicate of bug 357820 ***
