From 53ce39135cec31537b6dc25ebb3d23121700654f Mon Sep 17 00:00:00 2001 From: Goober5000 Date: Thu, 2 Apr 2026 01:23:34 -0400 Subject: [PATCH] add parent field to sexp_node for O(1) parent lookups The sexp_node struct used first/rest pointers but had no parent pointer, requiring O(n) linear scans of the entire Sexp_nodes array to find a node's parent. Add an int parent field (-1 for root/unlinked) and maintain it at all assignment sites: alloc_sexp, free_sexp, get_sexp parse loop, and save_branch. Refactor find_sexp_list(), find_parent_operator(), and is_sexp_top_level() to use the parent field directly; and add find_sexp_antecedent() and find_sexp_root(). Also add parent_node guards to several functions to allow them to gracefully fail on nodes with no parents. Also add documentation on how SEXP nodes and lists work, particularly the gotchas for top-level nodes. Co-Authored-By: Claude Opus 4.6 (1M context) --- code/missioneditor/sexp_tree_model.cpp | 6 +- code/missioneditor/sexp_tree_opf.cpp | 30 ++-- code/parse/sexp.cpp | 191 +++++++++++++++++++++---- code/parse/sexp.h | 34 ++++- 4 files changed, 221 insertions(+), 40 deletions(-) diff --git a/code/missioneditor/sexp_tree_model.cpp b/code/missioneditor/sexp_tree_model.cpp index e9827a81c55..2ce269c6ef3 100644 --- a/code/missioneditor/sexp_tree_model.cpp +++ b/code/missioneditor/sexp_tree_model.cpp @@ -694,6 +694,7 @@ int SexpTreeModel::save_branch(int cur, int at_root) const start = node; } else if (last >= 0) { Sexp_nodes[last].rest = node; + Sexp_nodes[node].parent = Sexp_nodes[last].parent; } last = node; @@ -767,8 +768,9 @@ int SexpTreeModel::find_ancestral_argument_number(int parent_op, int child_node) // which makes the special string a valid value at this position. bool SexpTreeModel::is_node_eligible_for_special_argument(int parent_node) const { - Assertion(parent_node != -1, - "Attempt to access invalid parent node for special arg eligibility check. Please report!"); + // if there's no parent, it's certainly not eligible + if (parent_node < 0) + return false; const int w_arg = find_ancestral_argument_number(OP_WHEN_ARGUMENT, parent_node); const int e_arg = find_ancestral_argument_number(OP_EVERY_TIME_ARGUMENT, parent_node); diff --git a/code/missioneditor/sexp_tree_opf.cpp b/code/missioneditor/sexp_tree_opf.cpp index dc29f09fa04..76052ffbc7b 100644 --- a/code/missioneditor/sexp_tree_opf.cpp +++ b/code/missioneditor/sexp_tree_opf.cpp @@ -361,7 +361,9 @@ sexp_list_item *SexpTreeOPF::get_listing_opf_ai_goal(int parent_node) const { sexp_list_item head; - Assertion(parent_node >= 0, "Invalid parent node"); + // no parent context, nothing to list + if (parent_node < 0) + return nullptr; int child = _model.tree_nodes[parent_node].child; if (child < 0) return nullptr; @@ -414,7 +416,9 @@ sexp_list_item *SexpTreeOPF::get_listing_opf_docker_point(int parent_node, int a { sexp_list_item head; - Assertion(parent_node >= 0, "Invalid parent node"); + // no parent context, nothing to list + if (parent_node < 0) + return nullptr; Assertion(!stricmp(_model.tree_nodes[parent_node].text, "ai-dock") || !stricmp(_model.tree_nodes[parent_node].text, "set-docked") || get_operator_const(_model.tree_nodes[parent_node].text) >= static_cast(First_available_operator_id), "Invalid node type"); @@ -479,7 +483,9 @@ sexp_list_item *SexpTreeOPF::get_listing_opf_dockee_point(int parent_node) const { sexp_list_item head; - Assertion(parent_node >= 0, "Invalid parent node"); + // no parent context, nothing to list + if (parent_node < 0) + return nullptr; Assertion(!stricmp(_model.tree_nodes[parent_node].text, "ai-dock") || !stricmp(_model.tree_nodes[parent_node].text, "set-docked"), "Invalid node type"); int sh = -1; @@ -765,7 +771,9 @@ sexp_list_item *SexpTreeOPF::get_listing_opf_goal_name(int parent_node) const { sexp_list_item head; - Assertion(parent_node >= 0, "Invalid parent node"); + // no parent context, nothing to list + if (parent_node < 0) + return nullptr; int child = _model.tree_nodes[parent_node].child; // reference_name is used by campaign editor to filter goals for a specific mission @@ -843,7 +851,9 @@ sexp_list_item *SexpTreeOPF::get_listing_opf_event_name(int parent_node) const { sexp_list_item head; - Assertion(parent_node >= 0, "Invalid parent node"); + // no parent context, nothing to list + if (parent_node < 0) + return nullptr; int child = _model.tree_nodes[parent_node].child; // reference_name is used by campaign editor to filter events for a specific mission @@ -1396,7 +1406,9 @@ sexp_list_item *SexpTreeOPF::get_listing_opf_animation_name(int parent_node) con { sexp_list_item head; - Assertion(parent_node >= 0, "Invalid parent node"); + // no parent context, nothing to list + if (parent_node < 0) + return nullptr; // get the operator type of the node const int op = get_operator_const(_model.tree_nodes[parent_node].text); @@ -1593,11 +1605,13 @@ enum : int { sexp_list_item *SexpTreeOPF::get_listing_opf_subsystem(int parent_node, int arg_index) const { sexp_list_item head; - + + // no parent context, nothing to list + if (parent_node < 0) + return nullptr; // determine if the parent is one of the set subsystem strength items. If so, // we want to append the "Hull" name onto the end of the menu - Assertion(parent_node >= 0, "Invalid parent node"); // get the operator type of the node int op = get_operator_const(_model.tree_nodes[parent_node].text); diff --git a/code/parse/sexp.cpp b/code/parse/sexp.cpp index 56bf8ad5434..270a968eaed 100644 --- a/code/parse/sexp.cpp +++ b/code/parse/sexp.cpp @@ -1475,6 +1475,7 @@ int alloc_sexp(const char *text, int type, int subtype, int first, int rest) Sexp_nodes[node].subtype = subtype; Sexp_nodes[node].first = first; Sexp_nodes[node].rest = rest; + Sexp_nodes[node].parent = -1; Sexp_nodes[node].value = SEXP_UNKNOWN; Sexp_nodes[node].flags = SNF_DEFAULT_VALUE; Sexp_nodes[node].op_index = NO_OPERATOR_INDEX_DEFINED; @@ -1482,6 +1483,18 @@ int alloc_sexp(const char *text, int type, int subtype, int first, int rest) Sexp_nodes[node].cached_variable_index = -1; Sexp_nodes[node].duration_index = -1; + // if this node was created with a first child, set the parent of the + // entire rest chain (the child and all its siblings) + if (first != -1 && first != Locked_sexp_true && first != Locked_sexp_false) + { + int child = first; + while (child != -1) + { + Sexp_nodes[child].parent = node; + child = Sexp_nodes[child].rest; + } + } + // special-arg? if (type == SEXP_ATOM && !strcmp(text, SEXP_ARGUMENT_STRING)) Sexp_nodes[node].flags |= SNF_SPECIAL_ARG_IN_NODE; @@ -1629,7 +1642,11 @@ int free_sexp(int num, int calling_node) if (calling_node >= 0) { if (Sexp_nodes[calling_node].first == num) + { Sexp_nodes[calling_node].first = rest; + if (rest != -1) + Sexp_nodes[rest].parent = calling_node; + } if (Sexp_nodes[calling_node].rest == num) Sexp_nodes[calling_node].rest = rest; @@ -1710,9 +1727,16 @@ int verify_sexp_tree(int node) } /** + * Duplicate a sexp chain rooted at @p node, returning the new root index. + * + * @p parent_for_root is the parent index that should be stamped onto the + * returned head and every sibling reachable via the head's `.rest` chain. + * Pass the index of the node whose `.first` or `.rest` slot will receive the + * duplicate, or -1 for a free-standing top-level duplicate. + * * @todo CASE OF SEXP VARIABLES - ONLY 1 COPY OF VARIABLE */ -int dup_sexp_chain(int node) +int dup_sexp_chain(int node, int parent_for_root) { int cur, first, rest; @@ -1721,8 +1745,8 @@ int dup_sexp_chain(int node) } // TODO - CASE OF SEXP VARIABLES - ONLY 1 COPY OF VARIABLE - first = dup_sexp_chain(Sexp_nodes[node].first); - rest = dup_sexp_chain(Sexp_nodes[node].rest); + first = dup_sexp_chain(Sexp_nodes[node].first, -1); + rest = dup_sexp_chain(Sexp_nodes[node].rest, parent_for_root); cur = alloc_sexp(Sexp_nodes[node].text, Sexp_nodes[node].type, Sexp_nodes[node].subtype, first, rest); if (cur == -1) { @@ -1732,8 +1756,10 @@ int dup_sexp_chain(int node) if (rest != -1){ free_sexp(rest); } + return -1; } + Sexp_nodes[cur].parent = parent_for_root; return cur; } @@ -1790,83 +1816,191 @@ int query_node_in_sexp(int node, int sexp) } /** - * Find the index of the list associated with an operator + * Find the node that contains the given node as its first element. */ int find_sexp_list(int num) { - int i; + if (num < 0 || num >= Num_sexp_nodes) + return -1; - for (i = 0; i < Num_sexp_nodes; i++) + // quick check via the parent, to narrow the search + int p = Sexp_nodes[num].parent; + if (p >= 0) + return (Sexp_nodes[p].first == num) ? p : -1; + + // longer check, since parent == -1 is ambiguous + // (see is_sexp_top_level) + for (int i = 0; i < Num_sexp_nodes; i++) { + if (Sexp_nodes[i].type == SEXP_NOT_USED || i == num) + continue; if (Sexp_nodes[i].first == num) return i; } - // not found return -1; } +/** + * Find the node that contains the given node as its rest element. + */ +int find_sexp_antecedent(int num) +{ + if (num < 0 || num >= Num_sexp_nodes) + return -1; + + // quick check via the parent, to narrow the search + int p = Sexp_nodes[num].parent; + if (p >= 0) + { + // a chain head is referenced via its parent's first, not via any rest; + // it legitimately has no antecedent + if (Sexp_nodes[p].first == num) + return -1; + + // look through the "rest" chain starting from the first child + for (int n = Sexp_nodes[p].first; n >= 0; n = Sexp_nodes[n].rest) + { + if (Sexp_nodes[n].rest == num) + return n; + } + + // If we get here, the parent field is stale or wrong: a node with parent == p + // must be reachable via p's first-child chain. Nodes in top-level rest chains + // (which have no wrapping list node) must have parent == -1 per the parser's + // convention; an antecedent that points to this node via rest while our parent + // is some other node means the invariant was broken at an assignment site. + Assertion(false, "find_sexp_antecedent: node %d has parent %d, but is not in that node's first-child chain!", num, p); + return -1; + } + + // longer check, since parent == -1 is ambiguous + // (see is_sexp_top_level) + for (int i = 0; i < Num_sexp_nodes; i++) + { + if (Sexp_nodes[i].type == SEXP_NOT_USED || i == num) + continue; + if (Sexp_nodes[i].rest == num) + return i; + } + + return -1; +} + +/** + * Find the root node of the SEXP tree that contains the given node. + */ +int find_sexp_root(int node) +{ + if (node < 0) + return -1; + + // visit each parent + while (Sexp_nodes[node].parent >= 0) + node = Sexp_nodes[node].parent; + + // this node's parent is -1, so we need to do one more loop (see also is_sexp_top_level) + for (int i = 0; i < Num_sexp_nodes; i++) + { + if ((Sexp_nodes[i].type == SEXP_NOT_USED) || (i == node)) + continue; + + if ((Sexp_nodes[i].first == node) || (Sexp_nodes[i].rest == node)) + return find_sexp_root(i); + } + + // no references: we've reached the true root + return node; +} + /** * Find node of operator that item is an argument of. */ int find_parent_operator(int node) { - int i; - Assert((node >= 0) && (node < Num_sexp_nodes)); + if (node < 0 || node >= Num_sexp_nodes) + return -1; + // operators are enclosed within a list, and the list will be the argument if (Sexp_nodes[node].subtype == SEXP_ATOM_OPERATOR) { node = find_sexp_list(node); // are we already at the top of the list? this will happen for non-standard sexps // (sexps that fire instantly instead of using a conditional) such as: - // $Formula: ( do-nothing ) + // $Formula: ( do-nothing ) if (node < 0) return -1; } - // iterate backwards through the sexps nodes (i.e. do the inverse of CDR) - while (Sexp_nodes[node].subtype != SEXP_ATOM_OPERATOR) + // quick check + int p = Sexp_nodes[node].parent; + if (p >= 0) { - for (i = 0; i < Num_sexp_nodes; i++) + int op = Sexp_nodes[p].first; + if (op < 0 || Sexp_nodes[op].subtype != SEXP_ATOM_OPERATOR) + return -1; + + // look through the "rest" chain of this operator + for (int n = Sexp_nodes[op].rest; n >= 0; n = Sexp_nodes[n].rest) { - if (Sexp_nodes[i].rest == node) - break; + if (n == node) + return op; } - if (i == Num_sexp_nodes) - return -1; // not found, probably at top node already. + return -1; + } - node = i; + // no parent, so we need to reverse-iterate until we find the operator + int n = node, a; + while (true) + { + a = find_sexp_antecedent(n); + if (a < 0) + break; + n = a; } - return node; + if (Sexp_nodes[n].subtype == SEXP_ATOM_OPERATOR) + return n; + else + return -1; } /** * Determine if an sexpression node is the top level node of an sexpression tree. * - * Top level nodes do not have their node id in anyone elses first or rest index. + * Top level nodes do not have their node id in anyone else's first or rest index. + * Note: we can't simply check parent == -1, because top-level chains from get_sexp_main() + * link operator and argument nodes via rest without a wrapping list node. */ -int is_sexp_top_level( int node ) +bool is_sexp_top_level(int node) { - int i; - Assert((node >= 0) && (node < Num_sexp_nodes)); if (Sexp_nodes[node].type == SEXP_NOT_USED) - return 0; + return false; - for (i = 0; i < Num_sexp_nodes; i++) + // fast path: if the node has a parent, it's definitely not top-level + if (Sexp_nodes[node].parent != -1) + return false; + + // parent == -1 is ambiguous: it could be a true root, or a sibling in a + // top-level rest chain (from get_sexp_main) that has no wrapping list node. + // Fall back to a full scan to distinguish the two cases. + // (This isn't too catastrophic, because the vast majority of nodes use the + // fast path, and also because the is_sexp_top_level() function is only used + // for mission post-processing and in update_sexp_references() in FRED.) + for (int i = 0; i < Num_sexp_nodes; i++) { - if ((Sexp_nodes[i].type == SEXP_NOT_USED) || (i == node )) // don't check myself or unused nodes + if ((Sexp_nodes[i].type == SEXP_NOT_USED) || (i == node)) continue; if ((Sexp_nodes[i].first == node) || (Sexp_nodes[i].rest == node)) - return 0; + return false; } - return 1; + return true; } /** @@ -4844,6 +4978,7 @@ int get_sexp() node = alloc_sexp("", SEXP_LIST, SEXP_ATOM_LIST, node, -1); } Sexp_nodes[last].rest = node; + Sexp_nodes[node].parent = Sexp_nodes[last].parent; if (message != nullptr) { SCP_string context; diff --git a/code/parse/sexp.h b/code/parse/sexp.h index bbc6a198f3a..609ae84b92d 100644 --- a/code/parse/sexp.h +++ b/code/parse/sexp.h @@ -1388,6 +1388,33 @@ struct sexp_cached_data } }; +// SEXP nodes use a Lisp-like structure where `first` is CAR (first child) and +// `rest` is CDR (next sibling). A sub-expression like ( op arg1 arg2 ) is +// represented as a SEXP_LIST node whose `first` points to the operator atom, +// with arguments chained via `rest`: +// +// ListNode (SEXP_LIST, text="") +// first -> OpAtom (SEXP_ATOM_OPERATOR, text="op") +// rest -> Arg1 (SEXP_ATOM, text="arg1") +// rest -> Arg2 (SEXP_ATOM, text="arg2") +// rest -> -1 +// +// Nested sub-expressions become SEXP_LIST nodes in the rest chain: +// +// ListNode (first -> OpAtom) +// OpAtom (rest -> InnerList) +// InnerList (SEXP_LIST, first -> InnerOp) +// InnerOp (rest -> InnerArg1 -> ...) +// +// The `parent` field points to the SEXP_LIST node that contains the chain. +// All siblings in a rest chain share the same parent. For example, OpAtom, +// Arg1, and Arg2 above all have parent == ListNode. +// +// Top-level sexps (from get_sexp_main) are a special case: the outermost +// operator and its arguments form a bare rest chain with no wrapping list +// node, so all nodes in that chain have parent == -1. This means parent == -1 +// is ambiguous: it could be a true root or a sibling in a top-level chain. +// See is_sexp_top_level() for how this is handled. typedef struct sexp_node { char text[TOKEN_LENGTH]; int op_index; // the index in the Operators array for the operator at this node (or -1 if not an operator) @@ -1395,6 +1422,7 @@ typedef struct sexp_node { int subtype; // type of atom or list? int first; // if first parameter is sexp, index into Sexp_nodes int rest; // index into Sexp_nodes of rest of parameters + int parent; // index into Sexp_nodes of parent node, or -1 if root/unlinked int value; // known to be true, known to be false, or not known int flags; // Goober5000 @@ -1463,11 +1491,13 @@ extern int find_free_sexp(); extern int free_one_sexp(int num); extern int free_sexp(int num, int calling_node = -1); extern int free_sexp2(int num, int calling_node = -1); -extern int dup_sexp_chain(int node); +extern int dup_sexp_chain(int node, int parent_for_root = -1); extern int cmp_sexp_chains(int node1, int node2); extern int find_sexp_list(int num); +extern int find_sexp_antecedent(int num); +extern int find_sexp_root(int node); extern int find_parent_operator(int num); -extern int is_sexp_top_level( int node ); +extern bool is_sexp_top_level( int node ); // Goober5000 - renamed these to be more clear, to prevent bugs :p extern int get_operator_index(const char *token);