View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0003136 | FSSCP | SEXPs | public | 2014-12-20 10:37 | 2015-05-01 06:34 |
Reporter | niffiwan | Assigned To | MageKing17 | ||
Priority | normal | Severity | minor | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Product Version | 3.7.2 RC4 | ||||
Summary | 0003136: number-of (and similar) never return SEXP_KNOWN_FALSE | ||||
Description | Per the summary. I came across the issue when trying to use when-argument + number-of + is-cargo-known-delay in a mission objective. I noticed that the objective didn't get marked as false when the ships to be scanned left the mission. This would seem to occur because eval_sexp never returns SEXP_KNOWN_FALSE (or SEXP_KNOWN_TRUE) despite what the comments say. It stores the KNOWN result in the sexp node then returns the equivalent non-KNOWN result. And then in a number of places in sexp.cpp the return value of eval_sexp is checked vs SEXP_KNOWN_FALSE or SEXP_KNOWN_TRUE. This is obviously an edge case, since usually (I believe) the KNOWN results are just used to save time/computation. And FRED2 doesn't let you use when/when-argument directly in mission objectives, you have to link to a different event using is-event-true-delay (or similar). And finally you need to be doing something oddball like wanting an objective to complete when you've scanned HALF the container group... | ||||
Steps To Reproduce | See attached test case (uses retail assets). Scan one of the when-arg containers to complete one objective Scan both the norm containers to complete the other objective Press 1/2 to destroy the containers per the directives When the containers are destroyed it's expected that the incomplete objectives will fail (per the behaviour of the norm group) | ||||
Additional Information | I've attached a patch where I've gone through sexp.cpp and "cleaned up" any place that checked the return value from eval_sexp against SEXP_KNOWN_TRUE or SEXP_KNOWN_FALSE. In many cases this merely resulted in removing the check since it was redundant. Another solution would be to do what the comments at the end of eval_sexp say, and actually return SEXP_KNOWN_TRUE or SEXP_KNOWN_FALSE. I welcome comment/discussion about this! Also; thanks to MageKing17 for noticing that my 1st solution was rubbish! | ||||
Tags | No tags attached. | ||||
|
mantis3136-svn.patch (4,006 bytes)
Index: code/parse/sexp.cpp =================================================================== --- code/parse/sexp.cpp (revision 11201) +++ code/parse/sexp.cpp (working copy) @@ -6241,7 +6241,7 @@ replace_current_value = eval_sexp(n); } - if ((replace_current_value == SEXP_KNOWN_FALSE) || (replace_current_value == SEXP_FALSE) ) { + if (replace_current_value == SEXP_FALSE) { // note: any SEXP_KNOWN_FALSE result will return SEXP_FALSE Directive_count += directive_value; } else { @@ -8366,12 +8366,8 @@ } // return whatever val was, but don't return known-* - if (val == SEXP_KNOWN_TRUE) - return SEXP_TRUE; - else if (val == SEXP_KNOWN_FALSE) - return SEXP_FALSE; - else - return val; + // note: SEXP_KNOWN_TRUE/SEXP_KNOWN_FALSE are never returned from eval_sexp + return val; } /** @@ -8409,7 +8405,7 @@ // if value is true, perform the actions in the 'then' part - if (val == SEXP_TRUE || val == SEXP_KNOWN_TRUE) + if (val == SEXP_TRUE) // note: SEXP_KNOWN_TRUE is never returned from eval_sexp { // get the operator int exp = CAR(actions); @@ -8441,7 +8437,7 @@ } } // if-then-else has actions to perform under "else" - else if ((val == SEXP_FALSE || val == SEXP_KNOWN_FALSE) && when_op_num == OP_IF_THEN_ELSE) + else if (val == SEXP_FALSE && when_op_num == OP_IF_THEN_ELSE) // note: SEXP_KNOWN_FALSE is never returned from eval_sexp { // skip past the "if" action actions = CDR(actions); @@ -8459,10 +8455,8 @@ } // invert val so that we behave like a when with opposite results - if (val == SEXP_KNOWN_FALSE) - val = SEXP_KNOWN_TRUE; - else - val = SEXP_TRUE; + // note: SEXP_KNOWN_FALSE is never returned from eval_sexp + val = SEXP_TRUE; } if (is_blank_argument_op(when_op_num)) @@ -8487,8 +8481,7 @@ if (Sexp_nodes[short_circuit_node].value == SEXP_KNOWN_FALSE) return SEXP_KNOWN_FALSE; // no need to waste time on this anymore - if (val == SEXP_KNOWN_FALSE) - return SEXP_FALSE; // can't return known false, as this would bypass future actions under the when + // note: val can't be SEXP_KNOWN_FALSE at this point return val; } @@ -8509,7 +8502,7 @@ // if the conditional evaluated to true, then we must evaluate the rest of the expression returning // the value of this evaluation - if (val == SEXP_TRUE || val == SEXP_KNOWN_TRUE) + if (val == SEXP_TRUE) // note: any SEXP_KNOWN_TRUE result is returned as SEXP_TRUE { int actions, exp; @@ -8565,6 +8558,10 @@ // evaluate conditional for current argument Sexp_replacement_arguments.push_back(Sexp_nodes[n].text); val = eval_sexp(condition_node); + if ( Sexp_nodes[condition_node].value == SEXP_KNOWN_TRUE || + Sexp_nodes[condition_node].value == SEXP_KNOWN_FALSE) { + val = Sexp_nodes[condition_node].value; + } switch (val) { @@ -8639,6 +8636,10 @@ // evaluate conditional for current argument Sexp_replacement_arguments.push_back(argument_vector[i]); val = eval_sexp(condition_node); + if ( Sexp_nodes[condition_node].value == SEXP_KNOWN_TRUE || + Sexp_nodes[condition_node].value == SEXP_KNOWN_FALSE) { + val = Sexp_nodes[condition_node].value; + } switch (val) { @@ -8839,7 +8840,7 @@ val = eval_sexp(condition_node); // true? - if (val == SEXP_TRUE || val == SEXP_KNOWN_TRUE) + if (val == SEXP_TRUE || Sexp_nodes[condition_node].value == SEXP_KNOWN_TRUE) { Sexp_applicable_argument_list.add_data(Sexp_nodes[n].text); } @@ -8888,7 +8889,7 @@ val = eval_sexp(condition_node); // true? - if (val == SEXP_TRUE || val == SEXP_KNOWN_TRUE) + if (val == SEXP_TRUE || Sexp_nodes[condition_node].value == SEXP_KNOWN_TRUE) { Sexp_applicable_argument_list.add_data(Sexp_nodes[n].text); } @@ -22210,7 +22211,7 @@ { int result = eval_sexp(cur_node, referenced_node); - return ((result == SEXP_TRUE) || (result == SEXP_KNOWN_TRUE)); + return (result == SEXP_TRUE); // note: any SEXP_KNOWN_TRUE result will return SEXP_TRUE } |
|
|
|
I've attached a (very) slightly modified version; it just removes the immediately-contradicted portion of the comment in eval_sexp() and makes test_argument_nodes_for_condition() also check for SEXP_NAN_FOREVER (treating it as SEXP_KNOWN_FALSE). EDIT: And test_argument_vector_for_condition(). |
|
3136.patch (4,798 bytes)
Index: code/parse/sexp.cpp =================================================================== --- code/parse/sexp.cpp (revision 11201) +++ code/parse/sexp.cpp (working copy) @@ -6241,7 +6241,7 @@ replace_current_value = eval_sexp(n); } - if ((replace_current_value == SEXP_KNOWN_FALSE) || (replace_current_value == SEXP_FALSE) ) { + if (replace_current_value == SEXP_FALSE) { // note: any SEXP_KNOWN_FALSE result will return SEXP_FALSE Directive_count += directive_value; } else { @@ -8366,12 +8366,8 @@ } // return whatever val was, but don't return known-* - if (val == SEXP_KNOWN_TRUE) - return SEXP_TRUE; - else if (val == SEXP_KNOWN_FALSE) - return SEXP_FALSE; - else - return val; + // note: SEXP_KNOWN_TRUE/SEXP_KNOWN_FALSE are never returned from eval_sexp + return val; } /** @@ -8409,7 +8405,7 @@ // if value is true, perform the actions in the 'then' part - if (val == SEXP_TRUE || val == SEXP_KNOWN_TRUE) + if (val == SEXP_TRUE) // note: SEXP_KNOWN_TRUE is never returned from eval_sexp { // get the operator int exp = CAR(actions); @@ -8441,7 +8437,7 @@ } } // if-then-else has actions to perform under "else" - else if ((val == SEXP_FALSE || val == SEXP_KNOWN_FALSE) && when_op_num == OP_IF_THEN_ELSE) + else if (val == SEXP_FALSE && when_op_num == OP_IF_THEN_ELSE) // note: SEXP_KNOWN_FALSE is never returned from eval_sexp { // skip past the "if" action actions = CDR(actions); @@ -8459,10 +8455,8 @@ } // invert val so that we behave like a when with opposite results - if (val == SEXP_KNOWN_FALSE) - val = SEXP_KNOWN_TRUE; - else - val = SEXP_TRUE; + // note: SEXP_KNOWN_FALSE is never returned from eval_sexp + val = SEXP_TRUE; } if (is_blank_argument_op(when_op_num)) @@ -8487,8 +8481,7 @@ if (Sexp_nodes[short_circuit_node].value == SEXP_KNOWN_FALSE) return SEXP_KNOWN_FALSE; // no need to waste time on this anymore - if (val == SEXP_KNOWN_FALSE) - return SEXP_FALSE; // can't return known false, as this would bypass future actions under the when + // note: val can't be SEXP_KNOWN_FALSE at this point return val; } @@ -8509,7 +8502,7 @@ // if the conditional evaluated to true, then we must evaluate the rest of the expression returning // the value of this evaluation - if (val == SEXP_TRUE || val == SEXP_KNOWN_TRUE) + if (val == SEXP_TRUE) // note: any SEXP_KNOWN_TRUE result is returned as SEXP_TRUE { int actions, exp; @@ -8565,6 +8558,13 @@ // evaluate conditional for current argument Sexp_replacement_arguments.push_back(Sexp_nodes[n].text); val = eval_sexp(condition_node); + if ( Sexp_nodes[condition_node].value == SEXP_KNOWN_TRUE || + Sexp_nodes[condition_node].value == SEXP_KNOWN_FALSE) { + val = Sexp_nodes[condition_node].value; + } else if ( Sexp_nodes[condition_node].value == SEXP_NAN_FOREVER ) { + // In accordance with SEXP_NAN/SEXP_NAN_FOREVER becoming SEXP_FALSE in eval_sexp() + val = SEXP_KNOWN_FALSE; + } switch (val) { @@ -8639,6 +8639,13 @@ // evaluate conditional for current argument Sexp_replacement_arguments.push_back(argument_vector[i]); val = eval_sexp(condition_node); + if ( Sexp_nodes[condition_node].value == SEXP_KNOWN_TRUE || + Sexp_nodes[condition_node].value == SEXP_KNOWN_FALSE) { + val = Sexp_nodes[condition_node].value; + } else if ( Sexp_nodes[condition_node].value == SEXP_NAN_FOREVER ) { + // In accordance with SEXP_NAN/SEXP_NAN_FOREVER becoming SEXP_FALSE in eval_sexp() + val = SEXP_KNOWN_FALSE; + } switch (val) { @@ -8839,7 +8846,7 @@ val = eval_sexp(condition_node); // true? - if (val == SEXP_TRUE || val == SEXP_KNOWN_TRUE) + if (val == SEXP_TRUE || Sexp_nodes[condition_node].value == SEXP_KNOWN_TRUE) { Sexp_applicable_argument_list.add_data(Sexp_nodes[n].text); } @@ -8888,7 +8895,7 @@ val = eval_sexp(condition_node); // true? - if (val == SEXP_TRUE || val == SEXP_KNOWN_TRUE) + if (val == SEXP_TRUE || Sexp_nodes[condition_node].value == SEXP_KNOWN_TRUE) { Sexp_applicable_argument_list.add_data(Sexp_nodes[n].text); } @@ -22210,7 +22217,7 @@ { int result = eval_sexp(cur_node, referenced_node); - return ((result == SEXP_TRUE) || (result == SEXP_KNOWN_TRUE)); + return (result == SEXP_TRUE); // note: any SEXP_KNOWN_TRUE result will return SEXP_TRUE } @@ -24637,7 +24644,7 @@ // if we haven't returned, check the sexp value of the sexpression evaluation. A special // value of known true or known false means that we should set the sexp.value field for - // short circuit eval (and return that special value as well). + // short circuit eval. if (sexp_val == SEXP_KNOWN_TRUE) { Sexp_nodes[cur_node].value = SEXP_KNOWN_TRUE; return SEXP_TRUE; |
|
So it turns out that released missions rely on the broken behavior that the various *-of SEXPs never short-circuit. For instance, in Her Finest Hour (from Act 3 of Blue Planet: War in Heaven), there's a repeating event that uses when-argument with an every-of with a condition of ( true ) that relies on that when-argument being evaluated every time the event fires. With this patch, it successfully short-circuits to SEXP_KNOWN_TRUE after the first evaluation and never executes again. Arguably, this is a problem with the mission: if it needs to be evaluated every time it's encountered, there's already a SEXP for that: every-time-argument. At the same time, the broken behavior is relied upon by a pivotal mission of a major mod that's been out (and working) for years at this point. If we adopt this patch out of the blue, it suddenly stops working because of a new FSO version, and the argument that it was really the FREDer's fault in the first place is... unsatisfying. At the same time, tying this to a game_settings.tbl setting seems convoluted (and would just make the relevant code even more complicated). Thoughts? EDIT: Scratch that, I'm a tired idiot. When doesn't short-circuit on SEXP_KNOWN_TRUE, so the *-of SEXPs shouldn't either (since their evaluation is required to have any arguments to process). Hopefully I'll have an updated patch up shortly. |
|
Wound up changing a few extra things while I was at it. Fixed potential memory leak in eval_for_counter() by freeing the duplicated strings after they're used. Also changed from strdup() to vm_strdup(), because we have our own versions of these functions for a reason. Tried to check for SEXP_NAN_FOREVER in appropriate places when checking SEXP_KNOWN_FALSE, since it has similar applications for short-circuiting when further evaluation is useless. Speaking of SEXP_NAN_FOREVER, made eval_sexp() bail early if the value is SEXP_NAN_FOREVER, just like SEXP_KNOWN_TRUE and SEXP_KNOWN_FALSE. Added SEXP_KNOWN_FALSE short-circuiting to the argument SEXPs that were lacking them at the same time as I was stripping out SEXP_KNOWN_TRUE short-circuiting for the ones that had it. Made every-time and every-time-argument not flush the SEXP tree if it evaluates to SEXP_KNOWN_FALSE; if the condition is returning SEXP_KNOWN_FALSE, continued evaluation is a waste of processing time. |
|
Turns out I'd accidentally broken random-of. Patch updated. |
|
I'll take a look at this tomorrow. However: "Made every-time and every-time-argument not flush the SEXP tree if it evaluates to SEXP_KNOWN_FALSE; if the condition is returning SEXP_KNOWN_FALSE, continued evaluation is a waste of processing time." That should be taken out. I know it's a waste of time, but it's part of the contract of the sexp that it should always be flushed and re-evaluated. And it's possible that there might be some side-effect of SEXP_KNOWN_FALSE that needs to be bypassed using every-time. (I can think of one already: let's say every-time-argument is evaluating a variable which contains the name of a destroyed ship. It might return SEXP_KNOWN_FALSE. Later on, the variable is changed to the name of a ship that does exist. The sexp needs to still be evaluated to handle this case.) |
|
sexp.cpp.patch (12,548 bytes)
Index: code/parse/sexp.cpp =================================================================== --- code/parse/sexp.cpp (revision 11328) +++ code/parse/sexp.cpp (working copy) @@ -4392,7 +4392,7 @@ if (CAR(n) != -1) { result &= is_sexp_true(CAR(n)); - if ( Sexp_nodes[CAR(n)].value == SEXP_KNOWN_FALSE ) + if ( Sexp_nodes[CAR(n)].value == SEXP_KNOWN_FALSE || Sexp_nodes[CAR(n)].value == SEXP_NAN_FOREVER ) return SEXP_KNOWN_FALSE; // if one of the AND clauses is FALSE, whole clause is false if ( Sexp_nodes[CAR(n)].value != SEXP_KNOWN_TRUE ) // if the value is still unknown, they all can't be true all_true = 0; @@ -4406,7 +4406,7 @@ new_result = is_sexp_true(CDR(n)); result &= new_result; - if ( Sexp_nodes[CDR(n)].value == SEXP_KNOWN_FALSE ) + if ( Sexp_nodes[CDR(n)].value == SEXP_KNOWN_FALSE || Sexp_nodes[CDR(n)].value == SEXP_NAN_FOREVER ) return SEXP_KNOWN_FALSE; // if one of the AND clauses is FALSE, whole clause is false if ( Sexp_nodes[CDR(n)].value != SEXP_KNOWN_TRUE ) // if the value is still unknown, they all can't be true all_true = 0; @@ -4435,7 +4435,7 @@ if (CAR(n) != -1) { result &= is_sexp_true(CAR(n)); - if ( Sexp_nodes[CAR(n)].value == SEXP_KNOWN_FALSE ) + if ( Sexp_nodes[CAR(n)].value == SEXP_KNOWN_FALSE || Sexp_nodes[CAR(n)].value == SEXP_NAN_FOREVER ) return SEXP_KNOWN_FALSE; // if one of the AND clauses is FALSE, whole clause is false if ( Sexp_nodes[CAR(n)].value != SEXP_KNOWN_TRUE ) // if value is true, mark our all_true variable for later checking all_true = 0; @@ -4458,7 +4458,7 @@ if ( next_result && !result ) // if current result is true, and our running result is false, thngs didn't become true in order return SEXP_KNOWN_FALSE; result &= next_result; - if ( Sexp_nodes[CDR(n)].value == SEXP_KNOWN_FALSE ) + if ( Sexp_nodes[CDR(n)].value == SEXP_KNOWN_FALSE || Sexp_nodes[CDR(n)].value == SEXP_NAN_FOREVER ) return SEXP_KNOWN_FALSE; // if one of the OR clauses is TRUE, whole clause is true if ( Sexp_nodes[CDR(n)].value != SEXP_KNOWN_TRUE ) // if the value is still unknown, they all can't be false all_true = 0; @@ -4490,14 +4490,12 @@ if (CAR(n) != -1) { result = is_sexp_true(CAR(n)); - if ( Sexp_nodes[CAR(n)].value == SEXP_KNOWN_FALSE ) + if ( Sexp_nodes[CAR(n)].value == SEXP_KNOWN_FALSE || Sexp_nodes[CDR(n)].value == SEXP_NAN_FOREVER ) return SEXP_KNOWN_TRUE; // not KNOWN_FALSE == KNOWN_TRUE; else if ( Sexp_nodes[CAR(n)].value == SEXP_KNOWN_TRUE ) // not KNOWN_TRUE == KNOWN_FALSE return SEXP_KNOWN_FALSE; else if ( Sexp_nodes[CAR(n)].value == SEXP_NAN ) // not NAN == TRUE (I think) return SEXP_TRUE; - else if ( Sexp_nodes[CAR(n)].value == SEXP_NAN_FOREVER ) - return SEXP_TRUE; } else result = atoi(CTEXT(n)); @@ -6242,7 +6240,7 @@ replace_current_value = eval_sexp(n); } - if ((replace_current_value == SEXP_KNOWN_FALSE) || (replace_current_value == SEXP_FALSE) ) { + if (replace_current_value == SEXP_FALSE) { // note: any SEXP_KNOWN_FALSE result will return SEXP_FALSE Directive_count += directive_value; } else { @@ -8367,12 +8365,8 @@ } // return whatever val was, but don't return known-* - if (val == SEXP_KNOWN_TRUE) - return SEXP_TRUE; - else if (val == SEXP_KNOWN_FALSE) - return SEXP_FALSE; - else - return val; + // note: SEXP_KNOWN_TRUE/SEXP_KNOWN_FALSE are never returned from eval_sexp + return val; } /** @@ -8410,7 +8404,7 @@ // if value is true, perform the actions in the 'then' part - if (val == SEXP_TRUE || val == SEXP_KNOWN_TRUE) + if (val == SEXP_TRUE) // note: SEXP_KNOWN_TRUE is never returned from eval_sexp { // get the operator int exp = CAR(actions); @@ -8442,7 +8436,7 @@ } } // if-then-else has actions to perform under "else" - else if ((val == SEXP_FALSE || val == SEXP_KNOWN_FALSE) && when_op_num == OP_IF_THEN_ELSE) + else if (val == SEXP_FALSE && when_op_num == OP_IF_THEN_ELSE) // note: SEXP_KNOWN_FALSE is never returned from eval_sexp { // skip past the "if" action actions = CDR(actions); @@ -8460,10 +8454,8 @@ } // invert val so that we behave like a when with opposite results - if (val == SEXP_KNOWN_FALSE) - val = SEXP_KNOWN_TRUE; - else - val = SEXP_TRUE; + // note: SEXP_KNOWN_FALSE is never returned from eval_sexp + val = SEXP_TRUE; } if (is_blank_argument_op(when_op_num)) @@ -8485,11 +8477,10 @@ // thanks to MageKing17 for noticing that we need to short-circuit on the correct node! int short_circuit_node = (arg_handler >= 0) ? arg_handler : cond; - if (Sexp_nodes[short_circuit_node].value == SEXP_KNOWN_FALSE) + if (Sexp_nodes[short_circuit_node].value == SEXP_KNOWN_FALSE || Sexp_nodes[short_circuit_node].value == SEXP_NAN_FOREVER) return SEXP_KNOWN_FALSE; // no need to waste time on this anymore - if (val == SEXP_KNOWN_FALSE) - return SEXP_FALSE; // can't return known false, as this would bypass future actions under the when + // note: val can't be SEXP_KNOWN_FALSE at this point return val; } @@ -8510,7 +8501,7 @@ // if the conditional evaluated to true, then we must evaluate the rest of the expression returning // the value of this evaluation - if (val == SEXP_TRUE || val == SEXP_KNOWN_TRUE) + if (val == SEXP_TRUE) // note: any SEXP_KNOWN_TRUE result is returned as SEXP_TRUE { int actions, exp; @@ -8566,6 +8557,13 @@ // evaluate conditional for current argument Sexp_replacement_arguments.push_back(Sexp_nodes[n].text); val = eval_sexp(condition_node); + if ( Sexp_nodes[condition_node].value == SEXP_KNOWN_TRUE || + Sexp_nodes[condition_node].value == SEXP_KNOWN_FALSE) { + val = Sexp_nodes[condition_node].value; + } else if ( Sexp_nodes[condition_node].value == SEXP_NAN_FOREVER ) { + // In accordance with SEXP_NAN/SEXP_NAN_FOREVER becoming SEXP_FALSE in eval_sexp() + val = SEXP_KNOWN_FALSE; + } switch (val) { @@ -8640,6 +8638,13 @@ // evaluate conditional for current argument Sexp_replacement_arguments.push_back(argument_vector[i]); val = eval_sexp(condition_node); + if ( Sexp_nodes[condition_node].value == SEXP_KNOWN_TRUE || + Sexp_nodes[condition_node].value == SEXP_KNOWN_FALSE) { + val = Sexp_nodes[condition_node].value; + } else if ( Sexp_nodes[condition_node].value == SEXP_NAN_FOREVER ) { + // In accordance with SEXP_NAN/SEXP_NAN_FOREVER becoming SEXP_FALSE in eval_sexp() + val = SEXP_KNOWN_FALSE; + } switch (val) { @@ -8705,12 +8710,10 @@ num_valid_arguments = test_argument_nodes_for_condition(n, condition_node, &num_true, &num_false, &num_known_true, &num_known_false); // use the sexp_or algorithm - if (num_known_true) - return SEXP_KNOWN_TRUE; + if (num_known_true || num_true) + return SEXP_TRUE; else if (num_known_false == num_valid_arguments) return SEXP_KNOWN_FALSE; - else if (num_true) - return SEXP_TRUE; else return SEXP_FALSE; } @@ -8730,8 +8733,6 @@ // use the sexp_and algorithm if (num_known_false) return SEXP_KNOWN_FALSE; - else if (num_known_true == num_valid_arguments) - return SEXP_KNOWN_TRUE; else if (num_false) return SEXP_FALSE; else @@ -8756,12 +8757,10 @@ // use the sexp_or algorithm, modified // (true if at least threshold arguments are true) - if (num_known_true >= threshold) - return SEXP_KNOWN_TRUE; + if (num_true + num_known_true >= threshold) + return SEXP_TRUE; else if (num_valid_arguments - num_known_false < threshold) return SEXP_KNOWN_FALSE; - else if (num_true + num_known_true >= threshold) - return SEXP_TRUE; else return SEXP_FALSE; } @@ -8773,9 +8772,17 @@ // addendum: hook karajorma's random-multiple-of into the same sexp int eval_random_of(int arg_handler_node, int condition_node, bool multiple) { - int n = -1, i, val, num_valid_args, random_argument; + int n = -1, i, val, num_valid_args, random_argument, num_known_false = 0; Assert(arg_handler_node != -1 && condition_node != -1); + // get the number of valid arguments + num_valid_args = query_sexp_args_count(arg_handler_node, true); + + if (num_valid_args == 0) + { + return SEXP_KNOWN_FALSE; // Not much point in trying to evaluate it again. + } + // find which argument we picked, if we picked one if (!multiple) { @@ -8794,32 +8801,32 @@ { n = CDR(arg_handler_node); - // get the number of valid arguments - num_valid_args = query_sexp_args_count(arg_handler_node, true); - if (num_valid_args == 0) - { - return SEXP_FALSE; - } - // pick an argument and iterate to it random_argument = rand_internal(1, num_valid_args); i = 0; - while (true) + for (int j = 0; j >= num_valid_args; n = CDR(n)) { Assert(n >= 0); // count only valid arguments - if (Sexp_nodes[n].flags & SNF_ARGUMENT_VALID) - i++; + if (Sexp_nodes[n].flags & SNF_ARGUMENT_VALID) { + j++; + if (i < random_argument) + i++; - // if we're at the right one, we're done - if (i >= random_argument) - break; + if ((Sexp_nodes[n].value == SEXP_KNOWN_FALSE) || (Sexp_nodes[n].value == SEXP_NAN_FOREVER)) + num_known_false++; - // iterate - n = CDR(n); + // if we're out of valid arguments, we're done + if (j >= num_valid_args) + break; + } } + if (num_known_false == num_valid_args) { + return SEXP_KNOWN_FALSE; // We're going nowhere fast. + } + // save it, if we're saving if (!multiple) { @@ -8840,9 +8847,11 @@ val = eval_sexp(condition_node); // true? - if (val == SEXP_TRUE || val == SEXP_KNOWN_TRUE) + if (val == SEXP_TRUE) { Sexp_applicable_argument_list.add_data(Sexp_nodes[n].text); + } else if ((!multiple || num_valid_args == 1) && (Sexp_nodes[condition_node].value == SEXP_KNOWN_FALSE || Sexp_nodes[condition_node].value == SEXP_NAN_FOREVER)) { + val = SEXP_KNOWN_FALSE; // If we can't randomly pick another one and this one is guaranteed never to be true, then give up now. } // clear argument, but not list, as we'll need it later @@ -8889,9 +8898,11 @@ val = eval_sexp(condition_node); // true? - if (val == SEXP_TRUE || val == SEXP_KNOWN_TRUE) + if (val == SEXP_TRUE) { Sexp_applicable_argument_list.add_data(Sexp_nodes[n].text); + } else if ((Sexp_nodes[condition_node].value == SEXP_KNOWN_FALSE) || (Sexp_nodes[condition_node].value == SEXP_NAN_FOREVER)) { + val = SEXP_KNOWN_FALSE; // If we're wasting our time evaluating this ever again, just go ahead and short-circuit. } // clear argument, but not list, as we'll need it later @@ -8951,19 +8962,22 @@ for (i = counter_start; ((counter_step > 0) ? i <= counter_stop : i >= counter_stop); i += counter_step) { sprintf(buf, "%d", i); - argument_vector.push_back(strdup(buf)); + argument_vector.push_back(vm_strdup(buf)); } // test the whole argument vector num_valid_arguments = test_argument_vector_for_condition(argument_vector, true, condition_node, &num_true, &num_false, &num_known_true, &num_known_false); + while (!argument_vector.empty()) { + vm_free(argument_vector.back()); + argument_vector.pop_back(); + } + // use the sexp_or algorithm - if (num_known_true) - return SEXP_KNOWN_TRUE; + if (num_known_true || num_true) + return SEXP_TRUE; else if (num_known_false == num_valid_arguments) return SEXP_KNOWN_FALSE; - else if (num_true) - return SEXP_TRUE; else return SEXP_FALSE; } @@ -22216,7 +22230,7 @@ { int result = eval_sexp(cur_node, referenced_node); - return ((result == SEXP_TRUE) || (result == SEXP_KNOWN_TRUE)); + return (result == SEXP_TRUE); // note: any SEXP_KNOWN_TRUE result will return SEXP_TRUE } @@ -22466,7 +22480,13 @@ add_to_event_log_buffer(get_operator_index(cur_node), SEXP_KNOWN_FALSE); } return SEXP_FALSE; - } + } + else if (Sexp_nodes[cur_node].value == SEXP_NAN_FOREVER) { + if (Log_event) { + add_to_event_log_buffer(get_operator_index(cur_node), SEXP_NAN_FOREVER); + } + return SEXP_FALSE; + } if (Sexp_nodes[cur_node].first != -1) { node = CAR(cur_node); @@ -24647,7 +24667,7 @@ // if we haven't returned, check the sexp value of the sexpression evaluation. A special // value of known true or known false means that we should set the sexp.value field for - // short circuit eval (and return that special value as well). + // short circuit eval. if (sexp_val == SEXP_KNOWN_TRUE) { Sexp_nodes[cur_node].value = SEXP_KNOWN_TRUE; return SEXP_TRUE; |
|
Mmm, good point. every-time change removed from patch. |
|
Your changes look reasonable. I'm wary of short-circuiting on SEXP_NAN_FOREVER but your logic is sound and I can't think of a good reason for not doing so. I am also wary of removing things that catch when eval_sexp returns values that it never returns. This makes sense, but it's easy to think of a well-intentioned coder accidentally wreaking havoc in the future. I suggest adding an Assertion to eval_sexp to ensure that it doesn't start returning unexpected values in the future. I did notice a potential problem in your changes to random_of. Invalid arguments can become valid again later on, so don't short-circuit if they all happen to be invalid at a particular point in time. I believe this means you should revert almost all of your changes (all except for testing the results of eval_sexp) to the random-of function. |
|
Invalid arguments can become valid again, but AFAIK, only through SEXPs contained under that when-argument, so if they're all invalid, they can't actually be made valid again. I'm not sure where you want an Assertion to be put in eval_sexp() in order to guarantee its return value... |
|
I was thinking that if the only return statement is at the end of the function, you could put the Assertion right before the return. But that may not be the case. And yes, arguments can only be validated by their own sexps, but a "validate-argument" operator doesn't need to be part of an argument branch. Now it's possible you're right, but I don't want to risk someone coming up with a sexp that neither of us thought of and running into a problem. This seems to me like a "too clever" solution that violates Knuth's warning against premature optimization. The new version of random-of is less readable and trickier to understand than the old version. |
|
"I was thinking that if the only return statement is at the end of the function, you could put the Assertion right before the return. But that may not be the case." No, eval_sexp() returns in several places, and the returns are usually fed explicit values (e.g. "return SEXP_TRUE"), not variables that can be checked in an assertion. As for why the extra checks were removed... I actually originally argued for not only keeping (and expanding) the checks, but actually allowing eval_sexp() to return SEXP_KNOWN_TRUE and SEXP_KNOWN_FALSE. I defer to The_E's decision at the time: <@The_E> Okay, so way I see it, the _KNOWN_ values should stay inside eval_sexp. Everything else should trust eval_sexp to return the correct answer <MageKing17> So going with niffiwan's solution, then. "...arguments can only be validated by their own sexps, but a 'validate-argument' operator doesn't need to be part of an argument branch." Doesn't matter. It has to be part of the same when-argument, and without any valid arguments, none of those SEXPs will ever be executed, meaning that continued evaluation is pointless and SEXP_KNOWN_FALSE short-circuiting is exactly what we want. It's exactly the same kind of short-circuiting that every other *-of operator should do (e.g. if any-of is called with 0 arguments, it'll see that num_known_false == num_valid_arguments and return SEXP_KNOWN_FALSE). Actually, I see that every-of currently fails to check num_valid_arguments, so I'll add a check to its SEXP_KNOWN_FALSE branch, too. |
|
The other *-of operators do the same short-circuiting with num_valid_arguments? So the same argument should apply to them as well... but by the same token, they serve as counterexample to the same argument. All right, you've persuaded me. If the other sexps already worked that way, then people have been using that logic for quite some time without any issues. I think we are now good to go, pending your modification of every-of. |
|
This issue now has a pull request: https://github.com/scp-fs2open/fs2open.github.com/pull/42 |
|
Ha, now I need to get myself up to speed on git. :) |
|
Merge approved, so let's call this resolved. |
Date Modified | Username | Field | Change |
---|---|---|---|
2014-12-20 10:37 | niffiwan | New Issue | |
2014-12-20 10:39 | niffiwan | File Added: mantis3136-svn.patch | |
2014-12-20 10:39 | niffiwan | File Added: whenarg_knowntrue.fs2 | |
2014-12-20 10:41 | niffiwan | Assigned To | => niffiwan |
2014-12-20 10:41 | niffiwan | Status | new => assigned |
2014-12-20 10:41 | niffiwan | Status | assigned => code review |
2014-12-20 16:17 | MageKing17 | File Added: 3136.patch | |
2014-12-20 16:18 | MageKing17 | Note Added: 0016421 | |
2014-12-20 16:23 | MageKing17 | File Deleted: 3136.patch | |
2014-12-20 16:23 | MageKing17 | File Added: 3136.patch | |
2014-12-20 16:24 | MageKing17 | Note Edited: 0016421 | |
2015-04-23 08:01 | MageKing17 | Note Added: 0016652 | |
2015-04-23 10:56 | MageKing17 | Note Edited: 0016652 | |
2015-04-23 12:02 | MageKing17 | File Added: sexp.cpp.patch | |
2015-04-23 12:11 | MageKing17 | Note Added: 0016653 | |
2015-04-23 12:11 | MageKing17 | Assigned To | niffiwan => MageKing17 |
2015-04-23 23:36 | MageKing17 | File Deleted: sexp.cpp.patch | |
2015-04-23 23:37 | MageKing17 | File Added: sexp.cpp.patch | |
2015-04-23 23:37 | MageKing17 | Note Added: 0016660 | |
2015-04-24 03:16 | Goober5000 | Note Added: 0016664 | |
2015-04-24 03:24 | MageKing17 | File Deleted: sexp.cpp.patch | |
2015-04-24 03:24 | MageKing17 | File Added: sexp.cpp.patch | |
2015-04-24 03:24 | MageKing17 | Note Added: 0016665 | |
2015-04-26 02:38 | Goober5000 | Note Added: 0016675 | |
2015-04-26 02:43 | MageKing17 | Note Added: 0016676 | |
2015-04-26 03:41 | Goober5000 | Note Added: 0016677 | |
2015-04-26 07:42 | MageKing17 | Note Added: 0016678 | |
2015-04-26 21:49 | Goober5000 | Note Added: 0016679 | |
2015-04-27 09:55 | MageKing17 | Note Added: 0016682 | |
2015-04-28 00:45 | Goober5000 | Note Added: 0016683 | |
2015-05-01 02:30 | Goober5000 | Note Added: 0016686 | |
2015-05-01 02:30 | Goober5000 | Status | code review => resolved |
2015-05-01 06:33 | MageKing17 | Status | resolved => closed |
2015-05-01 06:33 | MageKing17 | Resolution | open => fixed |
2015-05-01 06:34 | MageKing17 | Status | closed => feedback |
2015-05-01 06:34 | MageKing17 | Resolution | fixed => reopened |
2015-05-01 06:34 | MageKing17 | Status | feedback => resolved |
2015-05-01 06:34 | MageKing17 | Resolution | reopened => fixed |