Skip to content

Commit

Permalink
don't short-circuit eval_sexp on any nodes in a when-argument tree
Browse files Browse the repository at this point in the history
This is a follow-up to scp-fs2open#2942.  That PR solved most of the short-circuiting issues with nested conditionals under when-argument, but it was still possible for the code to incorrectly short-circuit on a nested conditional that had `<argument>` in an action operator but not a condition operator.  The cleanest way to fix this is to not short-circuit on *any* nodes that are part of a when-argument tree.
  • Loading branch information
Goober5000 committed Apr 25, 2024
1 parent 289d0b6 commit 539d89c
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 22 deletions.
54 changes: 37 additions & 17 deletions code/parse/sexp.cpp
Expand Up @@ -1079,6 +1079,7 @@ void sexp_copy_variable_from_index(int node);
void sexp_copy_variable_between_indexes(int node);

int verify_vector(const char *text);
bool is_descendant_of_when_argument_op(int node);


#define ARG_ITEM_F_DUP (1<<0)
Expand Down Expand Up @@ -2402,18 +2403,7 @@ int check_sexp_syntax(int node, int return_type, int recursive, int *bad_node, s

// if this is the special argument, make sure it is being properly used
if (Sexp_nodes[node].flags & SNF_SPECIAL_ARG_IN_NODE) {
bool found = false;
z = node;
while (true) {
z = find_parent_operator(z);
if (z < 0) {
break;
}
if (is_when_argument_op(get_operator_const(z))) {
found = true;
break;
}
}
bool found = is_descendant_of_when_argument_op(node);

// if there is no argument operator higher in the tree, we have a problem
if (!found) {
Expand Down Expand Up @@ -10538,6 +10528,35 @@ void do_action_for_each_special_argument( int cur_node )
}
}

bool is_descendant_of_when_argument_op(int node)
{
// empty tree
if (node < 0)
return false;

// cached?
if (Sexp_nodes[node].flags & SNF_DESCENDANT_OF_WHEN_ARG_OP)
return true;
if (Sexp_nodes[node].flags & SNF_NOT_DESCENDANT_OF_WHEN_ARG_OP)
return false;

// see whether this node or any parent node qualifies
// (note that find_parent_operator() is expensive, so we want to cache any and all results)
bool result;
if (Sexp_nodes[node].type == SEXP_ATOM && Sexp_nodes[node].subtype == SEXP_ATOM_OPERATOR && is_when_argument_op(get_operator_const(node)))
result = true;
else
result = is_descendant_of_when_argument_op(find_parent_operator(node));

// cache this result
if (result)
Sexp_nodes[node].flags |= SNF_DESCENDANT_OF_WHEN_ARG_OP;
else
Sexp_nodes[node].flags |= SNF_NOT_DESCENDANT_OF_WHEN_ARG_OP;

return result;
}

// Goober5000
bool special_argument_appears_in_sexp_tree(int node)
{
Expand Down Expand Up @@ -27102,12 +27121,13 @@ int eval_sexp(int cur_node, int referenced_node)
type = SEXP_NODE_TYPE(cur_node);
Assert( (type == SEXP_LIST) || (type == SEXP_ATOM) );

// trap known true and known false sexpressions. We don't trap on SEXP_NAN sexpressions since
// they may yet evaluate to true or false. If the sexp depends on the special argument,
// we can't 'know' its value so we skip this behaviour.
// Trap known true and known false sexpressions. We don't trap on SEXP_NAN sexpressions since
// they may yet evaluate to true or false. If the sexp is part of a when-argument tree,
// we can't 'know' its value since the sexp nodes may be evaluated in different ways for
// different arguments, so we skip this behaviour.

// we want to log event values for KNOWN_X or FOREVER_X before returning
if (!special_argument_appears_in_sexp_tree(cur_node)) {
if (!is_descendant_of_when_argument_op(cur_node)) {
// we want to log event values for KNOWN_X or FOREVER_X before returning
if (Log_event && ((Sexp_nodes[cur_node].value == SEXP_KNOWN_TRUE) || (Sexp_nodes[cur_node].value == SEXP_KNOWN_FALSE) || (Sexp_nodes[cur_node].value == SEXP_NAN_FOREVER))) {
// if this is a node that has been assigned the value by short-circuiting,
// it might not be the operator that returned the value
Expand Down
7 changes: 2 additions & 5 deletions code/parse/sexp.h
Expand Up @@ -1360,6 +1360,8 @@ typedef struct sexp_node {
#define SNF_CHECKED_ARG_FOR_VAR (1<<5)
#define SNF_CHECKED_NODE_FOR_OPF_POSITIVE (1<<6)
#define SNF_NODE_IS_OPF_POSITIVE (1<<7)
#define SNF_DESCENDANT_OF_WHEN_ARG_OP (1<<8)
#define SNF_NOT_DESCENDANT_OF_WHEN_ARG_OP (1<<9)
#define SNF_DEFAULT_VALUE SNF_ARGUMENT_VALID

typedef struct sexp_variable {
Expand Down Expand Up @@ -1456,11 +1458,6 @@ extern int sexp_get_variable_index(int node);
extern int sexp_atoi(int node);
extern bool sexp_can_construe_as_integer(int node);

// Goober5000
void do_action_for_each_special_argument(int cur_node);
bool special_argument_appears_in_sexp_tree(int node);
bool special_argument_appears_in_sexp_list(int node);

// Goober5000 - for special-arg SEXPs
extern bool is_when_argument_op(int op_const);
extern bool is_argument_provider_op(int op_const);
Expand Down

0 comments on commit 539d89c

Please sign in to comment.