put pause_parse/unpause_parse in RAII

code/bmpman/bmpman.cpp

@@ -1349,14 +1349,13 @@ bool bm_load_and_parse_eff(const char *filename, int dir_type, int *nframes, int
 	memset(file_text, 0, sizeof(file_text));
 	memset(file_text_raw, 0, sizeof(file_text_raw));
 
-	// pause anything that may happen to be parsing right now
-	pause_parse();
-
 	try
 	{
-		// now start parsing the EFF
+		// read the EFF into our own buffers so we don't disturb anything that may happen to be parsing right now
 		read_file_text(filename, dir_type, file_text, file_text_raw);
-		reset_parse(file_text);
+
+		// pause any current parsing and start parsing the EFF; parsing unpauses when the guard goes out of scope
+		PauseParseGuard guard(file_text, filename);
 
 		required_string("$Type:");
 		stuff_string(ext, F_NAME, sizeof(ext));
@@ -1373,13 +1372,9 @@ bool bm_load_and_parse_eff(const char *filename, int dir_type, int *nframes, int
 	catch (const parse::ParseException& e)
 	{
 		mprintf(("BMPMAN: Unable to parse '%s'!  Error message = %s.\n", filename, e.what()));
-		unpause_parse();
 		return false;
 	}
 
-	// done with EFF so unpause parsing so whatever can continue
-	unpause_parse();
-
 	if (!stricmp(NOX("dds"), ext)) {
 		c_type = BM_TYPE_DDS;
 	} else if (!stricmp(NOX("tga"), ext)) {

code/model/modelread.cpp

@@ -433,9 +433,9 @@ bool get_user_vec3d_value(char *buf, vec3d *value, bool require_brackets, const
 	char closing_bracket = '\0';
 	bool success = false;
 
-	pause_parse();
-	Mp = buf;
-	snprintf(Current_filename, sizeof(Current_filename), "submodel %s on %s", submodel_name, filename);
+	char pseudo_filename[MAX_PATH_LEN];
+	snprintf(pseudo_filename, sizeof(pseudo_filename), "submodel %s on %s", submodel_name, filename);
+	PauseParseGuard guard(buf, pseudo_filename);
 
 	// Check if there's a missing line break before the next "$".
 	char end_separator = '\0';
@@ -450,8 +450,8 @@ bool get_user_vec3d_value(char *buf, vec3d *value, bool require_brackets, const
 	}
 
 	// Note that we can't simply return from within this block
-	// because we always need to call unpause_parse before we
-	// leave the function.  A one-iteration loop with break
+	// because we always need to revert the end_separator replacement
+	// before we leave the function.  A one-iteration loop with break
 	// statements allows the code to jump to the end.  Alternatively,
 	// goto could have been used.
 	do {
@@ -495,7 +495,6 @@ bool get_user_vec3d_value(char *buf, vec3d *value, bool require_brackets, const
 		*end_pos = end_separator;
 	}
 
-	unpause_parse();
 	return success;
 }
 
@@ -3275,9 +3274,8 @@ int model_load(const  char* filename, ship_info* sip, ErrorType error_type, bool
 	if (!VALID_FNAME(filename)) {
 		return -1;
 	}
-	pause_parse();
-	Mp = Parse_text;
-	strcpy_s(Current_filename, filename);
+	// not parsing anything from Parse_text, but this makes parse errors report the model filename
+	PauseParseGuard guard(Parse_text, filename);
 
 	TRACE_SCOPE(tracing::LoadModelFile);
 
@@ -3322,7 +3320,6 @@ int model_load(const  char* filename, ship_info* sip, ErrorType error_type, bool
 		}
 		Polygon_models[num] = NULL;
 
-		unpause_parse();
 		return -1;
 	}
 
@@ -3521,7 +3518,6 @@ int model_load(const  char* filename, ship_info* sip, ErrorType error_type, bool
 	model_set_subsys_path_nums(pm, n_subsystems, subsystems);
 	model_set_bay_path_nums(pm);
 
-	unpause_parse();
 	if (sip != nullptr)
 		sip->model_num = pm->id;
 	return pm->id;

code/parse/parselo.cpp

@@ -2376,6 +2376,11 @@ void allocate_parse_text(size_t size)
 {
 	Assert( size > 0 );
 
+	// while parsing is paused, the shared buffers are referenced by the bookmark
+	// stack (and Parse_text may not even point at them), so clearing or
+	// reallocating them would corrupt the paused parse
+	Assertion( Bookmarks.empty(), "allocate_parse_text() must not be called while parsing is paused!" );
+
 	// Make sure that there is space for the terminating null character
 	size += 1;
 
@@ -3638,29 +3643,54 @@ void find_and_stuff_or_add(const char *id, int *addr, int f_type, char *strlist[
 	}
 }
 
-// pause current parsing so that some else can be parsed without interfering
-// with the currently parsing file
-void pause_parse()
+PauseParseGuard::PauseParseGuard(char *text, const char *filename)
 {
-	Bookmark Mark;
+	// text may be null when nothing is being parsed, but a filename is always needed for error reporting
+	Assertion(filename != nullptr, "PauseParseGuard filename must not be null!");
 
+	// pause current parsing so that the supplied text can be parsed without interfering with the currently parsing file
+	Bookmark Mark;
 	Mark.filename = Current_filename;
+	Mark.Parse_text = Parse_text;
 	Mark.Mp = Mp;
 	Mark.Warning_count = Warning_count;
 	Mark.Error_count = Error_count;
-
 	Bookmarks.push_back(std::move(Mark));
+	m_depth = Bookmarks.size();
+
+	// point the parser at the new text.  Parse_text must track the start of the buffer containing Mp so that get_line_num() walks the right buffer.
+	// If text is the parser's current position (the lookahead idiom), the position is still within the current buffer, so the buffer start is
+	// unchanged; otherwise the supplied text becomes the new buffer.
+	if (text != Mp)
+		Parse_text = text;
+	Mp = text;
+	Warning_count = 0;
+	Error_count = 0;
+
+	// install the new filename, truncating if necessary; skip the copy when the caller passes Current_filename itself to bookmark the current position
+	if (filename != Current_filename)
+		snprintf(Current_filename, sizeof(Current_filename), "%s", filename);
 }
 
-// unpause parsing to continue with previously parsing file
-void unpause_parse()
+PauseParseGuard::~PauseParseGuard()
 {
-	Assert( !Bookmarks.empty() );
+	unpause();
+}
+
+// unpause parsing to continue with the previously parsing file
+void PauseParseGuard::unpause()
+{
+	if (!m_active)
+		return;
+	m_active = false;
+
+	Assertion(Bookmarks.size() == m_depth, "Bookmark stack mismatch: expected depth " SIZE_T_ARG " but found " SIZE_T_ARG ".  Something between pausing and unpausing left the stack unbalanced.", m_depth, Bookmarks.size());
 	if (Bookmarks.empty())
 		return;
 
-	Bookmark Mark = Bookmarks.back();
+	const Bookmark &Mark = Bookmarks.back();
 
+	Parse_text = Mark.Parse_text;
 	Mp = Mark.Mp;
 	Warning_count = Mark.Warning_count;
 	Error_count = Mark.Error_count;
@@ -3670,13 +3700,10 @@ void unpause_parse()
 	Bookmarks.pop_back();
 }
 
-void reset_parse(char *text)
+// restart parsing at the beginning of the parse text; to parse some other text, use PauseParseGuard
+void reset_parse()
 {
-	if (text != NULL) {
-		Mp = text;
-	} else {
-		Mp = Parse_text;
-	}
+	Mp = Parse_text;
 
 	Warning_count = 0;
 	Error_count = 0;

code/parse/parselo.h

@@ -348,10 +348,38 @@ extern void parse_int_list(int *ilist, size_t size);
 extern void parse_string_map(SCP_map<SCP_string, SCP_string>& mapOut, const char* end_marker, const char* entry_prefix);
 
 // general
-extern void reset_parse(char *text = NULL);
+extern void reset_parse();
 extern void display_parse_diagnostics();
-extern void pause_parse();
-extern void unpause_parse();
+
+// RAII guard that pauses the current parse so that some other text can be parsed
+// without interfering with the currently parsing file.  The constructor bookmarks
+// the current parse state (buffer, position, filename, and warning/error counts)
+// on a stack, then points the parser at the supplied text and filename with zeroed
+// counts.  The destructor (or an explicit unpause() call) restores the bookmarked
+// state, even if the parse path exits via an exception.  Guards nest in LIFO order.
+//
+// To bookmark the current position for lookahead, pass the parser's own state:
+//     PauseParseGuard guard(Mp, Current_filename);
+// Unpausing rewinds the parser to the bookmarked position.
+class PauseParseGuard
+{
+public:
+	PauseParseGuard(char *text, const char *filename);
+	~PauseParseGuard();
+
+	// restore the bookmarked parse state now, rather than at end of scope;
+	// subsequent calls (including the destructor's) are no-ops
+	void unpause();
+
+	PauseParseGuard(const PauseParseGuard &) = delete;
+	PauseParseGuard &operator=(const PauseParseGuard &) = delete;
+	PauseParseGuard(PauseParseGuard &&) = delete;
+	PauseParseGuard &operator=(PauseParseGuard &&) = delete;
+
+private:
+	size_t m_depth = 0;
+	bool m_active = true;
+};
 // stop parsing, basically just frees up the memory from Parse_text and Parse_text_raw
 extern void stop_parse();
 
@@ -499,6 +527,7 @@ namespace parse
 	{
 	public:
 		SCP_string filename;
+		char* Parse_text;
 		char* Mp;
 		int Warning_count;
 		int Error_count;

code/parse/sexp/LuaAISEXP.cpp

@@ -51,7 +51,7 @@ bool LuaAISEXP::parseCheckEndOfDescription() {
 	// Since we're stuffing strings, this is the best way to "unstuff" a string
 	// if we determine we're finished with the description - and we also want
 	// to preserve whitespace (which the parser eats) while building the description
-	pause_parse();
+	PauseParseGuard guard(Mp, Current_filename);	// bookmark the current position for lookahead; rewinds when the guard goes out of scope
 
 	// look for any token that can follow $Description
 	auto possible_tokens =
@@ -76,7 +76,6 @@ bool LuaAISEXP::parseCheckEndOfDescription() {
 		}
 	}
 
-	unpause_parse();
 	return found;
 }
 

code/parse/sexp/LuaSEXP.cpp

@@ -537,7 +537,7 @@ bool LuaSEXP::parseCheckEndOfDescription() {
 	// Since we're stuffing strings, this is the best way to "unstuff" a string
 	// if we determine we're finished with the description - and we also want
 	// to preserve whitespace (which the parser eats) while building the description
-	pause_parse();
+	PauseParseGuard guard(Mp, Current_filename);	// bookmark the current position for lookahead; rewinds when the guard goes out of scope
 
 	// look for any token that can follow $Description
 	auto possible_tokens =
@@ -558,7 +558,6 @@ bool LuaSEXP::parseCheckEndOfDescription() {
 		}
 	}
 
-	unpause_parse();
 	return found;
 }
 void LuaSEXP::parseTable() {

code/scripting/scripting.cpp

@@ -935,9 +935,8 @@ bool script_state::ParseCondition(const char *filename)
 		sat.global_conditions = parsed_conditions;
 		for (const SCP_string& local_condition : conditions) {
 			bool found = false;
-			pause_parse();
 			SCP_vm_unique_ptr<char> parse{ vm_strdup(local_condition.c_str()) };
-			reset_parse(parse.get());	// coverity[escape:FALSE] - this is okay because the pointer escape only lasts until unpause_parse() restores the old state
+			PauseParseGuard guard(parse.get(), buf.c_str());	// coverity[escape:FALSE] - this is okay because the pointer escape only lasts until the guard restores the old state
 			for (const auto& potential_condition : currHook->_conditions) {
 				SCP_string bufCond;
 				sprintf(bufCond, "$%s:", potential_condition.first.c_str());
@@ -949,7 +948,7 @@ bool script_state::ParseCondition(const char *filename)
 					break;
 				}
 			}
-			unpause_parse();
+			guard.unpause();
 
 			if (!found) {
 				error_display(0, "Condition '%s' is not valid for hook '%s'. The hook will not evaluate!", local_condition.c_str(), currHook->getHookName().c_str());

code/ship/ship.cpp

@@ -3850,7 +3850,7 @@ static void parse_ship_values(ship_info* sip, const bool is_template, const bool
 
 		// figure out whether this is a supercap by doing some parse gymnastics
 		// (flags are parsed several lines later, so we need to skip ahead, peek at the flags, and jump back)
-		pause_parse();
+		PauseParseGuard guard(Mp, Current_filename);	// bookmark the current position for lookahead; rewinds on unpause
 		if (skip_to_string("$Flags:", "$Name:") == 1) {
 			// cache the flag definition so we don't have to keep looking it up
 			static auto supercap_flag_def = std::find_if(std::begin(Ship_flags), std::end(Ship_flags), [](const flag_def_list_new<Info_Flags> &item) {
@@ -3869,7 +3869,6 @@ static void parse_ship_values(ship_info* sip, const bool is_template, const bool
 				}
 			}
 		}
-		unpause_parse();
 	}
 
 	// get ship parameters for warpin and warpout

code/weapon/weapons.cpp

@@ -410,7 +410,7 @@ void parse_wi_flags(weapon_info *weaponp)
     // have to do this slightly out of order in order to handle spawn properly
     // skip OVER flags, check for +override and reset num_spawn_weapons_defined
     // otherwise if done afterward, new and old spawn weapons can get mixed up
-    pause_parse();
+    PauseParseGuard guard(Mp, Current_filename);	// bookmark the current position for lookahead; rewinds on unpause
     SCP_vector<SCP_string> flags;
     stuff_string_list(flags);
     if (optional_string("+override")) {
@@ -445,7 +445,7 @@ void parse_wi_flags(weapon_info *weaponp)
     	weaponp->wi_flags = cleared_wi_flags;
     	weaponp->num_spawn_weapons_defined = 0;
     }
-    unpause_parse();
+    guard.unpause();	// rewind so the flag list can be parsed for real
 
 	// To make sure +override doesn't overwrite previously parsed values we parse the flags into a separate flagset
     SCP_vector<SCP_string> unparsed;