View Issue Details

IDProjectCategoryView StatusLast Update
0003136FSSCPSEXPspublic2015-05-01 06:34
Reporterniffiwan Assigned ToMageKing17  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.7.2 RC4 
Summary0003136: number-of (and similar) never return SEXP_KNOWN_FALSE
DescriptionPer 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 ReproduceSee 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 InformationI'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!
TagsNo tags attached.

Activities

niffiwan

2014-12-20 10:39

developer  

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
 }
 
 
mantis3136-svn.patch (4,006 bytes)   

niffiwan

2014-12-20 10:39

developer  

whenarg_knowntrue.fs2 (5,936 bytes)

MageKing17

2014-12-20 16:18

developer   ~0016421

Last edited: 2014-12-20 16:24

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().

MageKing17

2014-12-20 16:23

developer  

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;
3136.patch (4,798 bytes)   

MageKing17

2015-04-23 08:01

developer   ~0016652

Last edited: 2015-04-23 10:56

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.

MageKing17

2015-04-23 12:11

developer   ~0016653

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.

MageKing17

2015-04-23 23:37

developer   ~0016660

Turns out I'd accidentally broken random-of. Patch updated.

Goober5000

2015-04-24 03:16

administrator   ~0016664

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.)

MageKing17

2015-04-24 03:24

developer  

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;
sexp.cpp.patch (12,548 bytes)   

MageKing17

2015-04-24 03:24

developer   ~0016665

Mmm, good point. every-time change removed from patch.

Goober5000

2015-04-26 02:38

administrator   ~0016675

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.

MageKing17

2015-04-26 02:43

developer   ~0016676

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...

Goober5000

2015-04-26 03:41

administrator   ~0016677

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.

MageKing17

2015-04-26 07:42

developer   ~0016678

"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.

Goober5000

2015-04-26 21:49

administrator   ~0016679

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.

MageKing17

2015-04-27 09:55

developer   ~0016682

This issue now has a pull request:

https://github.com/scp-fs2open/fs2open.github.com/pull/42

Goober5000

2015-04-28 00:45

administrator   ~0016683

Ha, now I need to get myself up to speed on git. :)

Goober5000

2015-05-01 02:30

administrator   ~0016686

Merge approved, so let's call this resolved.

Issue History

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