Codechange: Use std::variant to store string parameter data.

This avoids storing two separate values and makes the test for which type is held clearer.

This replaces use of unique_ptr for conditionally storing a string, and is also used in place of StringParameterBackup.

src/error.h

@@ -31,7 +31,7 @@ enum WarningLevel {
 class ErrorMessageData {
 protected:
 	bool is_critical;               ///< Whether the error message is critical.
-	std::vector<StringParameterBackup> params; ///< Backup of parameters of the message strings.
+	std::vector<StringParameterData> params; ///< Backup of parameters of the message strings.
 	const GRFFile *textref_stack_grffile; ///< NewGRF that filled the #TextRefStack for the error message.
 	uint textref_stack_size;        ///< Number of uint32_t values to put on the #TextRefStack for the error message.
 	uint32_t textref_stack[16];       ///< Values to put on the #TextRefStack for the error message.

src/misc_gui.cpp

@@ -668,7 +668,7 @@ static WindowDesc _tool_tips_desc(
 struct TooltipsWindow : public Window
 {
 	StringID string_id;               ///< String to display as tooltip.
-	std::vector<StringParameterBackup> params; ///< The string parameters.
+	std::vector<StringParameterData> params; ///< The string parameters.
 	TooltipCloseCondition close_cond; ///< Condition for closing the window.
 
 	TooltipsWindow(Window *parent, StringID str, uint paramcount, TooltipCloseCondition close_tooltip) : Window(_tool_tips_desc)
@@ -1089,7 +1089,7 @@ void ShowQueryString(StringID str, StringID caption, uint maxsize, Window *paren
  */
 struct QueryWindow : public Window {
 	QueryCallbackProc *proc; ///< callback function executed on closing of popup. Window* points to parent, bool is true if 'yes' clicked, false otherwise
-	std::vector<StringParameterBackup> params; ///< local copy of #_global_string_params
+	std::vector<StringParameterData> params; ///< local copy of #_global_string_params
 	StringID message;        ///< message shown for query window
 
 	QueryWindow(WindowDesc &desc, StringID caption, StringID message, Window *parent, QueryCallbackProc *callback) : Window(desc)

src/news_gui.cpp

@@ -296,7 +296,7 @@ struct NewsWindow : Window {
 		this->CreateNestedTree();
 
 		/* For company news with a face we have a separate headline in param[0] */
-		if (&desc == &_company_news_desc) this->GetWidget<NWidgetCore>(WID_N_TITLE)->widget_data = this->ni->params[0].data;
+		if (&desc == &_company_news_desc) this->GetWidget<NWidgetCore>(WID_N_TITLE)->widget_data = std::get<uint64_t>(this->ni->params[0]);
 
 		NWidgetCore *nwid = this->GetWidget<NWidgetCore>(WID_N_SHOW_GROUP);
 		if (ni->reftype1 == NR_VEHICLE && nwid != nullptr) {
@@ -603,7 +603,7 @@ private:
 	{
 		/* Company news with a face have a separate headline, so the normal message is shifted by two params */
 		CopyInDParam(std::span(this->ni->params.data() + 2, this->ni->params.size() - 2));
-		return this->ni->params[1].data;
+		return std::get<uint64_t>(this->ni->params[1]);
 	}
 
 	StringID GetNewVehicleMessageString(WidgetID widget) const
@@ -986,7 +986,7 @@ void ChangeVehicleNews(VehicleID from_index, VehicleID to_index)
 	for (auto &ni : _news) {
 		if (ni.reftype1 == NR_VEHICLE && ni.ref1 == from_index) ni.ref1 = to_index;
 		if (ni.reftype2 == NR_VEHICLE && ni.ref2 == from_index) ni.ref2 = to_index;
-		if (ni.flags & NF_VEHICLE_PARAM0 && ni.params[0].data == from_index) ni.params[0] = to_index;
+		if (ni.flags & NF_VEHICLE_PARAM0 && std::get<uint64_t>(ni.params[0]) == from_index) ni.params[0] = to_index;
 	}
 }
 

src/news_type.h

@@ -139,7 +139,7 @@ struct NewsItem {
 
 	std::unique_ptr<const NewsAllocatedData> data; ///< Custom data for the news item that will be deallocated (deleted) when the news item has reached its end.
 
-	std::vector<StringParameterBackup> params; ///< Parameters for string resolving.
+	std::vector<StringParameterData> params; ///< Parameters for string resolving.
 
 	NewsItem(StringID string_id, NewsType type, NewsFlag flags, NewsReferenceType reftype1, uint32_t ref1, NewsReferenceType reftype2, uint32_t ref2, const NewsAllocatedData *data);
 };

src/strings.cpp

@@ -113,7 +113,7 @@ void SetDParam(size_t n, uint64_t v)
  */
 uint64_t GetDParam(size_t n)
 {
-	return _global_string_params.GetParam(n);
+	return std::get<uint64_t>(_global_string_params.GetParam(n));
 }
 
 /**
@@ -156,15 +156,10 @@ void SetDParamMaxDigits(size_t n, uint count, FontSize size)
  * Copy the parameters from the backup into the global string parameter array.
  * @param backup The backup to copy from.
  */
-void CopyInDParam(const std::span<const StringParameterBackup> backup)
+void CopyInDParam(const std::span<const StringParameterData> backup)
 {
 	for (size_t i = 0; i < backup.size(); i++) {
-		auto &value = backup[i];
+		_global_string_params.SetParam(i, backup[i]);
-		if (value.string.has_value()) {
-			_global_string_params.SetParam(i, value.string.value());
-		} else {
-			_global_string_params.SetParam(i, value.data);
-		}
 	}
 }
 
@@ -173,16 +168,11 @@ void CopyInDParam(const std::span<const StringParameterBackup> backup)
  * @param backup The backup to write to.
  * @param num Number of string parameters to copy.
  */
-void CopyOutDParam(std::vector<StringParameterBackup> &backup, size_t num)
+void CopyOutDParam(std::vector<StringParameterData> &backup, size_t num)
 {
 	backup.resize(num);
 	for (size_t i = 0; i < backup.size(); i++) {
-		const char *str = _global_string_params.GetParamStr(i);
+		backup[i] = _global_string_params.GetParam(i);
-		if (str != nullptr) {
-			backup[i] = str;
-		} else {
-			backup[i] = _global_string_params.GetParam(i);
-		}
 	}
 }
 
@@ -191,20 +181,12 @@ void CopyOutDParam(std::vector<StringParameterBackup> &backup, size_t num)
  * @param backup The backup to check against.
  * @return True when the parameters have changed, otherwise false.
  */
-bool HaveDParamChanged(const std::span<const StringParameterBackup> backup)
+bool HaveDParamChanged(const std::span<const StringParameterData> backup)
 {
-	bool changed = false;
+	for (size_t i = 0; i < backup.size(); i++) {
-	for (size_t i = 0; !changed && i < backup.size(); i++) {
+		if (backup[i] != _global_string_params.GetParam(i)) return true;
-		bool global_has_string = _global_string_params.GetParamStr(i) != nullptr;
-		if (global_has_string != backup[i].string.has_value()) return true;
-
-		if (global_has_string) {
-			changed = backup[i].string.value() != _global_string_params.GetParamStr(i);
-		} else {
-			changed = backup[i].data != _global_string_params.GetParam(i);
-		}
 	}
-	return changed;
+	return false;
 }
 
 static void StationGetSpecialString(StringBuilder &builder, StationFacility x);
@@ -1106,7 +1088,7 @@ static void FormatString(StringBuilder &builder, const char *str_arg, StringPara
 				case SCC_PLURAL_LIST: { // {P}
 					int plural_form = *str++;          // contains the plural form for this string
 					size_t offset = orig_offset + (uint8_t)*str++;
-					int64_t v = args.GetParam(offset); // contains the number that determines plural
+					int64_t v = std::get<uint64_t>(args.GetParam(offset)); // contains the number that determines plural
 					str = ParseStringChoice(str, DeterminePluralForm(v, plural_form), builder);
 					break;
 				}

src/strings_func.h

@@ -98,9 +98,9 @@ void SetDParamStr(size_t n, const char *str);
 void SetDParamStr(size_t n, const std::string &str);
 void SetDParamStr(size_t n, std::string &&str);
 
-void CopyInDParam(const std::span<const StringParameterBackup> backup);
+void CopyInDParam(const std::span<const StringParameterData> backup);
-void CopyOutDParam(std::vector<StringParameterBackup> &backup, size_t num);
+void CopyOutDParam(std::vector<StringParameterData> &backup, size_t num);
-bool HaveDParamChanged(const std::span<const StringParameterBackup> backup);
+bool HaveDParamChanged(const std::span<const StringParameterData> backup);
 
 uint64_t GetDParam(size_t n);
 

src/strings_internal.h

@@ -15,8 +15,7 @@
 
 /** The data required to format and validate a single parameter of a string. */
 struct StringParameter {
-	uint64_t data; ///< The data of the parameter.
+	StringParameterData data; ///< The data of the parameter.
-	std::unique_ptr<std::string> string; ///< Copied string value, if it has any.
 	char32_t type; ///< The #StringControlCode to interpret this data with when it's the first parameter, otherwise '\0'.
 };
 
@@ -93,7 +92,9 @@ public:
 	T GetNextParameter()
 	{
 		const auto &param = GetNextParameterReference();
-		return static_cast<T>(param.data);
+		const uint64_t *data = std::get_if<uint64_t>(&param.data);
+		if (data != nullptr) return static_cast<T>(*data);
+		return T{};
 	}
 
 	/**
@@ -105,7 +106,9 @@ public:
 	const char *GetNextParameterString()
 	{
 		const auto &param = GetNextParameterReference();
-		return param.string != nullptr ? param.string->c_str() : nullptr;
+		const std::string *data = std::get_if<std::string>(&param.data);
+		if (data != nullptr) return data->c_str();
+		return nullptr;
 	}
 
 	/**
@@ -146,11 +149,16 @@ public:
 		return this->parameters[offset].type;
 	}
 
+	void SetParam(size_t n, const StringParameterData &v)
+	{
+		assert(n < this->parameters.size());
+		this->parameters[n].data = v;
+	}
+
 	void SetParam(size_t n, uint64_t v)
 	{
 		assert(n < this->parameters.size());
 		this->parameters[n].data = v;
-		this->parameters[n].string.reset();
 	}
 
 	template <typename T, std::enable_if_t<std::is_base_of<StrongTypedefBase, T>::value, int> = 0>
@@ -162,8 +170,7 @@ public:
 	void SetParam(size_t n, const char *str)
 	{
 		assert(n < this->parameters.size());
-		this->parameters[n].data = 0;
+		this->parameters[n].data = str;
-		this->parameters[n].string = std::make_unique<std::string>(str);
 	}
 
 	void SetParam(size_t n, const std::string &str) { this->SetParam(n, str.c_str()); }
@@ -171,28 +178,14 @@ public:
 	void SetParam(size_t n, std::string &&str)
 	{
 		assert(n < this->parameters.size());
-		this->parameters[n].data = 0;
+		this->parameters[n].data = std::move(str);
-		this->parameters[n].string = std::make_unique<std::string>(std::move(str));
 	}
 
-	uint64_t GetParam(size_t n) const
+	const StringParameterData &GetParam(size_t n) const
 	{
 		assert(n < this->parameters.size());
-		assert(this->parameters[n].string == nullptr);
 		return this->parameters[n].data;
 	}
-
-	/**
-	 * Get the stored string of the parameter, or \c nullptr when there is none.
-	 * @param n The index into the parameters.
-	 * @return The stored string.
-	 */
-	const char *GetParamStr(size_t n) const
-	{
-		assert(n < this->parameters.size());
-		auto &param = this->parameters[n];
-		return param.string != nullptr ? param.string->c_str() : nullptr;
-	}
 };
 
 /**

src/strings_type.h

@@ -88,34 +88,6 @@ enum SpecialStrings {
 	SPECSTR_PRESIDENT_NAME     = 0x70E7,
 };
 
-/** Data that is to be stored when backing up StringParameters. */
+using StringParameterData = std::variant<uint64_t, std::string>;
-struct StringParameterBackup {
-	uint64_t data; ///< The data field; valid *when* string has no value.
-	std::optional<std::string> string; ///< The string value.
-
-	/**
-	 * Assign the numeric data with the given value, while clearing the stored string.
-	 * @param data The new value of the data field.
-	 * @return This object.
-	 */
-	StringParameterBackup &operator=(uint64_t data)
-	{
-		this->string.reset();
-		this->data = data;
-		return *this;
-	}
-
-	/**
-	 * Assign a copy of the given string to the string field, while clearing the data field.
-	 * @param string The new value of the string.
-	 * @return This object.
-	 */
-	StringParameterBackup &operator=(const std::string_view string)
-	{
-		this->data = 0;
-		this->string.emplace(string);
-		return *this;
-	}
-};
 
 #endif /* STRINGS_TYPE_H */

src/tests/strings_func.cpp

@@ -18,7 +18,7 @@ TEST_CASE("HaveDParamChanged")
 	SetDParam(0, 0);
 	SetDParamStr(1, "some string");
 
-	std::vector<StringParameterBackup> backup;
+	std::vector<StringParameterData> backup;
 	CopyOutDParam(backup, 2);
 
 	CHECK(HaveDParamChanged(backup) == false);

src/texteff.cpp

@@ -21,7 +21,7 @@
 
 /** Container for all information about a text effect */
 struct TextEffect : public ViewportSign {
-	std::vector<StringParameterBackup> params; ///< Backup of string parameters
+	std::vector<StringParameterData> params; ///< Backup of string parameters
 	StringID string_id;  ///< String to draw for the text effect, if INVALID_STRING_ID then it's not valid
 	uint8_t duration;      ///< How long the text effect should stay, in ticks (applies only when mode == TE_RISING)
 	TextEffectMode mode; ///< Type of text effect