fix result propagation in constexpr operator chains #312
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## compile-time chained operator evaluation mechanism
Constants are stored in `value` object. The object has two members: `constval` and `boolresult`. The `constval` member stores the actual value which the object represents: `15` and `10` for example in `15 < 20`. All operators are in a chain of operators. On most occasions, there is just one operator in the chain but it is still a valid singleton chain. The `boolresult` member stores the result of the previous operations which the constant was involved in if any. For example, in `15 < 20 < 30`, the `boolresult` member of object representing `20` stores the result of `15 < 20`. In more complex expressions such as, `15 < 20 < 30 < 40 < 50`, the `boolresult` member of object representing `40` stores the result of the `15 < 20 < 30 < 40`.
The `boolresult` of the first constant in a chain is set to `TRUE` by default.
**Details:**
The processing of chains begins at `plnge_rel` which maintains two `value` objects: `lval` and `lval2`. The `lval` is taken by `plnge_rel` as an argument of the first constant (stored in `constval` field) value in the operator chain. The result of the full chain is also passed through it by storing the result in the `constval` field (like `15 < 20` evaluates to the constant `1`; the result of `15 < 20` is `1` which is also a constant).
The `boolresult` of `lval` is set to `TRUE` as this is the first constant in the chain. The `plnge_rel` progresses through the chain one by one with `lval` and `lval2` always being the left operand and the right operand respectively. For each operator it calls `plnge2` which dumps instructions for loading constant and the right operand into the staging buffer. If the right operand turns out to be a constant, the staging buffer is flushed and `static cell calc(cell left,void (*oper)(),cell right,char *boolresult);` is called to evaluate the compile-time expression with the constant value of the left and right operand passed as `left` and `right` arguments along with the operator and `boolresult` of the left operand. `calc` evluates the expression and returns the right operand and sets the `boolresult` to the result of the operator chain. After the call returns, `constval` of `lval` (left operand) is set to the `constval` of `lval2` (right operand). As of now, the `lval` now stores the right operand and the result of the operator chain uptil now.
`plnge2` returns and control is transfered back to `plnge_rel`. `plnge_rel` copies the value stored in `lval2` into `lval`. This implies that the `boolresult` and `constval` fields of `lval2` are copied into `lval` as the previous right operand is now the left operand for the next operator in the chain. **Oops! BUG!** The `boolresult` which was stored in `lval` is lost and is replaced by `FALSE` which was stored in `lval2` which is the default for `value` objets (set by `clear_value`). The compiler should've ensured that the `boolresult` of `lval` was retained. `plnge2` is again called to process the right operand and evaluate the expression.
This keeps repeating until the entire chain has been evaluated. After the entire chain has been evaluated, `lval` has `constval` as the most recent right operand and `boolresult` has the result of the entire chain. The compiler sets `constval` of `lval` to `boolresult` of `lval` and its tag to `bool`. Note that this is done in the last step. The `lval` now is a constant representing the result of the entire chain of operators, i.e. `1` if the chain evaluated to `TRUE` or `0` otherwise.
**Relavant bugged code:**
```
static int plnge_rel(int *opstr,int opoff,int (*hier)(value *lval),value *lval)
{
.
.
.
lval->boolresult=TRUE;
do {
/* same check as in plnge(), but "chkbitwise" is always TRUE */
if (count>0 && bitwise_opercount!=0)
error(212);
if (count>0) {
relop_prefix();
*lval=lval2; /* copy right hand expression of the previous iteration */
} /* if */
opidx+=opoff;
plnge2(op1[opidx],hier,lval,&lval2);
if (count++>0)
relop_suffix();
} while (nextop(&opidx,opstr)); /* enddo */
lval->constval=lval->boolresult;
lval->tag=pc_addtag("bool"); /* force tag to be "bool" */
return FALSE; /* result of expression is not an lvalue */
}
```
```
static void plnge2(void (*oper)(void),
int (*hier)(value *lval),
value *lval1,value *lval2)
{
.
.
.
if (check_userop(oper,lval1->tag,lval2->tag,2,NULL,&lval1->tag)) {
lval1->ident=iEXPRESSION;
lval1->constval=0;
} else if (lval1->ident==iCONSTEXPR && lval2->ident==iCONSTEXPR) {
/* only constant expression if both constant */
stgdel(index,cidx); /* scratch generated code and calculate */
check_tagmismatch(lval1->tag,lval2->tag,FALSE,-1);
lval1->constval=calc(lval1->constval,oper,lval2->constval,&lval1->boolresult);
} else {
.
.
.
}
```
```
static cell calc(cell left,void (*oper)(),cell right,char *boolresult)
{
.
.
.
else if (oper==os_le)
return *boolresult &= (char)(left <= right), right;
else if (oper==os_ge)
return *boolresult &= (char)(left >= right), right;
else if (oper==os_lt)
return *boolresult &= (char)(left < right), right;
else if (oper==os_gt)
return *boolresult &= (char)(left > right), right;
.
.
.
}
```
As you can see, the `boolresult` is ANDed with the result of the current operation. If one of the operators in the chain earlier had evaluated to false, the `&` will force `boolresult` to remain `FALSE` irrespective of whether the current expression evaluates to true or not.
### What's the bug?
The compiler loses the `boolresult` of `lval` in `plnge_rel` and replaces it with the `boolresult` of `lval2` which is always initalized to `FALSE` by `clear_val`.
The fix is to ensure that `lval` retains the value.
This commits does this by copying the `boolresult` of `lval2` into `lval` before `*lval=lval2`. This ensures that the `boolresult` is retained in `lval`.
YashasSamaga
changed the title
|
This PR is ready to be merged. Requires approval @pawn-lang/compiler @Daniel-Cortez |
Southclaws
approved these changes
Jul 27, 2018
|
I think appveyor got stuck (been running since this morning) so I just forced merge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
The compiler fails to propagate the result of chained operations through the entire chain.
15 < 20 < 30evaluates to false even though it is true.This PR resolves the above issue.
Refer to the commit description and #308 (comment) for details.
Which issue(s) this PR fixes:
Fixes #308
What kind of pull this is: