Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Codechange: use vector with struct for string parameter backups #11052

Merged
merged 5 commits into from
Jul 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/error.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@ enum WarningLevel {
class ErrorMessageData {
protected:
bool is_critical; ///< Whether the error message is critical.
uint64 decode_params[20]; ///< Parameters of the message strings.
const char *strings[20]; ///< Copies of raw strings that were used.
std::vector<StringParameterBackup> 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 values to put on the #TextRefStack for the error message.
uint32 textref_stack[16]; ///< Values to put on the #TextRefStack for the error message.
Expand All @@ -44,7 +43,6 @@ class ErrorMessageData {

public:
ErrorMessageData(const ErrorMessageData &data);
~ErrorMessageData();
ErrorMessageData(StringID summary_msg, StringID detailed_msg, bool is_critical = false, int x = 0, int y = 0, const GRFFile *textref_stack_grffile = nullptr, uint textref_stack_size = 0, const uint32 *textref_stack = nullptr, StringID extra_msg = INVALID_STRING_ID);

/* Remove the copy assignment, as the default implementation will not do the right thing. */
Expand Down
42 changes: 11 additions & 31 deletions src/error_gui.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -74,24 +74,10 @@ static WindowDesc _errmsg_face_desc(
* @param data The data to copy.
*/
ErrorMessageData::ErrorMessageData(const ErrorMessageData &data) :
is_critical(data.is_critical), textref_stack_grffile(data.textref_stack_grffile), textref_stack_size(data.textref_stack_size),
is_critical(data.is_critical), params(data.params), textref_stack_grffile(data.textref_stack_grffile), textref_stack_size(data.textref_stack_size),
summary_msg(data.summary_msg), detailed_msg(data.detailed_msg), extra_msg(data.extra_msg), position(data.position), face(data.face)
{
memcpy(this->textref_stack, data.textref_stack, sizeof(this->textref_stack));
memcpy(this->decode_params, data.decode_params, sizeof(this->decode_params));
memcpy(this->strings, data.strings, sizeof(this->strings));
for (size_t i = 0; i < lengthof(this->strings); i++) {
if (this->strings[i] != nullptr) {
this->strings[i] = stredup(this->strings[i]);
this->decode_params[i] = (size_t)this->strings[i];
}
}
}

/** Free all the strings. */
ErrorMessageData::~ErrorMessageData()
{
for (size_t i = 0; i < lengthof(this->strings); i++) free(this->strings[i]);
}

/**
Expand All @@ -118,9 +104,6 @@ ErrorMessageData::ErrorMessageData(StringID summary_msg, StringID detailed_msg,
this->position.x = x;
this->position.y = y;

memset(this->decode_params, 0, sizeof(this->decode_params));
memset(this->strings, 0, sizeof(this->strings));

if (textref_stack_size > 0) MemCpyT(this->textref_stack, textref_stack, textref_stack_size);

assert(summary_msg != INVALID_STRING_ID);
Expand All @@ -137,14 +120,9 @@ void ErrorMessageData::CopyOutDParams()
if (company < MAX_COMPANIES) face = company;
}

/* Reset parameters */
for (size_t i = 0; i < lengthof(this->strings); i++) free(this->strings[i]);
memset(this->decode_params, 0, sizeof(this->decode_params));
memset(this->strings, 0, sizeof(this->strings));

/* Get parameters using type information */
if (this->textref_stack_size > 0) StartTextRefStackUsage(this->textref_stack_grffile, this->textref_stack_size, this->textref_stack);
CopyOutDParam(this->decode_params, this->strings, this->detailed_msg == INVALID_STRING_ID ? this->summary_msg : this->detailed_msg, lengthof(this->decode_params));
CopyOutDParam(this->params, 20, this->detailed_msg == INVALID_STRING_ID ? this->summary_msg : this->detailed_msg);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic 20, presumably that's just some limit that was set elsewhere before.

Copy link
Contributor Author

@rubidium42 rubidium42 Jul 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, error.h:34/35 to be precise. In the end only the needed amount of parameters should be copied, but that's for later.

if (this->textref_stack_size > 0) StopTextRefStackUsage();
}

Expand All @@ -155,7 +133,8 @@ void ErrorMessageData::CopyOutDParams()
*/
void ErrorMessageData::SetDParam(uint n, uint64 v)
{
this->decode_params[n] = v;
if (n >= this->params.size()) this->params.resize(n + 1);
this->params[n] = v;
}

/**
Expand All @@ -165,8 +144,8 @@ void ErrorMessageData::SetDParam(uint n, uint64 v)
*/
void ErrorMessageData::SetDParamStr(uint n, const char *str)
{
free(this->strings[n]);
this->strings[n] = stredup(str);
if (n >= this->params.size()) this->params.resize(n + 1);
this->params[n] = str;
}

/**
Expand All @@ -176,7 +155,8 @@ void ErrorMessageData::SetDParamStr(uint n, const char *str)
*/
void ErrorMessageData::SetDParamStr(uint n, const std::string &str)
{
this->SetDParamStr(n, str.c_str());
if (n >= this->params.size()) this->params.resize(n + 1);
this->params[n] = str;
}

/** The actual queue with errors. */
Expand Down Expand Up @@ -212,7 +192,7 @@ struct ErrmsgWindow : public Window, ErrorMessageData {
{
switch (widget) {
case WID_EM_MESSAGE: {
CopyInDParam(this->decode_params, lengthof(this->decode_params));
CopyInDParam(this->params);
if (this->textref_stack_size > 0) StartTextRefStackUsage(this->textref_stack_grffile, this->textref_stack_size, this->textref_stack);

this->height_summary = GetStringHeight(this->summary_msg, size->width);
Expand Down Expand Up @@ -281,7 +261,7 @@ struct ErrmsgWindow : public Window, ErrorMessageData {

void SetStringParameters(int widget) const override
{
if (widget == WID_EM_CAPTION) CopyInDParam(this->decode_params, lengthof(this->decode_params));
if (widget == WID_EM_CAPTION) CopyInDParam(this->params);
}

void DrawWidget(const Rect &r, int widget) const override
Expand All @@ -294,7 +274,7 @@ struct ErrmsgWindow : public Window, ErrorMessageData {
}

case WID_EM_MESSAGE:
CopyInDParam(this->decode_params, lengthof(this->decode_params));
CopyInDParam(this->params);
if (this->textref_stack_size > 0) StartTextRefStackUsage(this->textref_stack_grffile, this->textref_stack_size, this->textref_stack);

if (this->detailed_msg == INVALID_STRING_ID) {
Expand Down
19 changes: 7 additions & 12 deletions src/misc_gui.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -662,17 +662,14 @@ static WindowDesc _tool_tips_desc(
struct TooltipsWindow : public Window
{
StringID string_id; ///< String to display as tooltip.
byte paramcount; ///< Number of string parameters in #string_id.
uint64 params[8]; ///< The string parameters.
std::vector<StringParameterBackup> 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)
{
this->parent = parent;
this->string_id = str;
assert(paramcount <= lengthof(this->params));
this->paramcount = paramcount;
if (paramcount > 0) CopyOutDParam(this->params, this->paramcount);
CopyOutDParam(this->params, paramcount);
this->close_cond = close_tooltip;

this->InitNested();
Expand Down Expand Up @@ -703,7 +700,7 @@ struct TooltipsWindow : public Window
void UpdateWidgetSize(int widget, Dimension *size, const Dimension &padding, Dimension *fill, Dimension *resize) override
{
/* There is only one widget. */
for (uint i = 0; i != this->paramcount; i++) SetDParam(i, this->params[i]);
CopyInDParam(this->params);

size->width = std::min<uint>(GetStringBoundingBox(this->string_id).width, ScaleGUITrad(194));
size->height = GetStringHeight(this->string_id, size->width);
Expand All @@ -719,9 +716,7 @@ struct TooltipsWindow : public Window
GfxFillRect(r, PC_BLACK);
GfxFillRect(r.Shrink(WidgetDimensions::scaled.bevel), PC_LIGHT_YELLOW);

for (uint arg = 0; arg < this->paramcount; arg++) {
SetDParam(arg, this->params[arg]);
}
CopyInDParam(this->params);
DrawStringMultiLine(r.Shrink(WidgetDimensions::scaled.framerect).Shrink(WidgetDimensions::scaled.fullbevel), this->string_id, TC_BLACK, SA_CENTER);
}

Expand Down Expand Up @@ -1098,14 +1093,14 @@ 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
uint64 params[10]; ///< local copy of #_global_string_params
std::vector<StringParameterBackup> 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)
{
/* Create a backup of the variadic arguments to strings because it will be
* overridden pretty often. We will copy these back for drawing */
CopyOutDParam(this->params, lengthof(this->params));
CopyOutDParam(this->params, 10);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic 10, as above I suppose.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, misc_gui.cpp:1101 to be precise.

this->message = message;
this->proc = callback;
this->parent = parent;
Expand Down Expand Up @@ -1134,7 +1129,7 @@ struct QueryWindow : public Window {
switch (widget) {
case WID_Q_CAPTION:
case WID_Q_TEXT:
CopyInDParam(this->params, lengthof(this->params));
CopyInDParam(this->params);
break;
}
}
Expand Down
16 changes: 8 additions & 8 deletions src/news_gui.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,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];
if (desc == &_company_news_desc) this->GetWidget<NWidgetCore>(WID_N_TITLE)->widget_data = this->ni->params[0].data;

NWidgetCore *nwid = this->GetWidget<NWidgetCore>(WID_N_SHOW_GROUP);
if (ni->reftype1 == NR_VEHICLE && nwid != nullptr) {
Expand Down Expand Up @@ -361,7 +361,7 @@ struct NewsWindow : Window {
break;

case WID_N_MESSAGE:
CopyInDParam(this->ni->params, lengthof(this->ni->params));
CopyInDParam(this->ni->params);
str = this->ni->string_id;
break;

Expand Down Expand Up @@ -429,7 +429,7 @@ struct NewsWindow : Window {
break;

case WID_N_MESSAGE:
CopyInDParam(this->ni->params, lengthof(this->ni->params));
CopyInDParam(this->ni->params);
DrawStringMultiLine(r.left, r.right, r.top, r.bottom, this->ni->string_id, TC_FROMSTRING, SA_CENTER);
break;

Expand Down Expand Up @@ -582,8 +582,8 @@ struct NewsWindow : Window {
StringID GetCompanyMessageString() const
{
/* Company news with a face have a separate headline, so the normal message is shifted by two params */
CopyInDParam(this->ni->params + 2, lengthof(this->ni->params) - 2);
return this->ni->params[1];
CopyInDParam(span(this->ni->params.data() + 2, this->ni->params.size() - 2));
return this->ni->params[1].data;
}

StringID GetNewVehicleMessageString(int widget) const
Expand Down Expand Up @@ -804,7 +804,7 @@ NewsItem::NewsItem(StringID string_id, NewsType type, NewsFlag flags, NewsRefere
{
/* show this news message in colour? */
if (TimerGameCalendar::year >= _settings_client.gui.coloured_news_year) this->flags |= NF_INCOLOUR;
CopyOutDParam(this->params, lengthof(this->params));
CopyOutDParam(this->params, 10);
}

/**
Expand Down Expand Up @@ -998,7 +998,7 @@ void ChangeVehicleNews(VehicleID from_index, VehicleID to_index)
for (NewsItem *ni = _oldest_news; ni != nullptr; ni = ni->next) {
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] == from_index) ni->params[0] = to_index;
if (ni->flags & NF_VEHICLE_PARAM0 && ni->params[0].data == from_index) ni->params[0] = to_index;
}
}

Expand Down Expand Up @@ -1099,7 +1099,7 @@ void ShowLastNewsMessage()
*/
static void DrawNewsString(uint left, uint right, int y, TextColour colour, const NewsItem *ni)
{
CopyInDParam(ni->params, lengthof(ni->params));
CopyInDParam(ni->params);

/* Get the string, replaces newlines with spaces and remove control codes from the string. */
std::string message = StrMakeValid(GetString(ni->string_id), SVS_REPLACE_TAB_CR_NL_WITH_SPACE);
Expand Down
2 changes: 1 addition & 1 deletion src/news_type.h
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,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.

uint64 params[10]; ///< Parameters for string resolving.
std::vector<StringParameterBackup> params; ///< Parameters for string resolving.

NewsItem(StringID string_id, NewsType type, NewsFlag flags, NewsReferenceType reftype1, uint32 ref1, NewsReferenceType reftype2, uint32 ref2, const NewsAllocatedData *data);
};
Expand Down
2 changes: 1 addition & 1 deletion src/statusbar_gui.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@

static bool DrawScrollingStatusText(const NewsItem *ni, int scroll_pos, int left, int right, int top, int bottom)
{
CopyInDParam(ni->params, lengthof(ni->params));
CopyInDParam(ni->params);

/* Replace newlines and the likes with spaces. */
std::string message = StrMakeValid(GetString(ni->string_id), SVS_REPLACE_TAB_CR_NL_WITH_SPACE);
Expand Down
50 changes: 28 additions & 22 deletions src/strings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -154,45 +154,51 @@ void SetDParamMaxDigits(size_t n, uint count, FontSize size)
}

/**
* Copy \a num string parameters from array \a src into the global string parameter array.
* @param src Source array of string parameters.
* @param num Number of string parameters to copy.
* Copy the parameters from the backup into the global string parameter array.
* @param backup The backup to copy from.
*/
void CopyInDParam(const uint64 *src, int num)
void CopyInDParam(const span<const StringParameterBackup> backup)
{
for (int i = 0; i < num; i++) SetDParam(i, src[i]);
for (size_t i = 0; i < backup.size(); i++) {
auto &value = backup[i];
if (value.string.has_value()) {
_global_string_params.SetParam(i, value.string.value());
} else {
_global_string_params.SetParam(i, value.data);
}
rubidium42 marked this conversation as resolved.
Show resolved Hide resolved
}
}

/**
* Copy \a num string parameters from the global string parameter array to the \a dst array.
* @param dst Destination array of string parameters.
* @param num Number of string parameters to copy.
* Copy \a num string parameters from the global string parameter array to the \a backup.
* @param backup The backup to write to.
* @param num Number of string parameters to copy.
*/
void CopyOutDParam(uint64 *dst, int num)
void CopyOutDParam(std::vector<StringParameterBackup> &backup, size_t num)
{
for (int i = 0; i < num; i++) dst[i] = GetDParam(i);
backup.resize(num);
for (size_t i = 0; i < backup.size(); i++) {
backup[i] = _global_string_params.GetParam(i);
}
}

/**
* Copy \a num string parameters from the global string parameter array to the \a dst array.
* Furthermore clone raw string parameters into \a strings and amend the data in \a dst.
* @param dst Destination array of string parameters.
* @param strings Destination array for clone of the raw strings. Must be of same length as dst. Deallocation left to the caller.
* @param string The string used to determine where raw strings are and where there are no raw strings.
* @param num Number of string parameters to copy.
* Copy \a num string parameters from the global string parameter array to the \a backup.
* @param backup The backup to write to.
* @param num Number of string parameters to copy.
* @param string The string used to determine where raw strings are and where there are no raw strings.
*/
void CopyOutDParam(uint64 *dst, const char **strings, StringID string, int num)
void CopyOutDParam(std::vector<StringParameterBackup> &backup, size_t num, StringID string)
{
/* Just get the string to extract the type information. */
GetString(string);

for (int i = 0; i < num; i++) {
backup.resize(num);
for (size_t i = 0; i < backup.size(); i++) {
if (_global_string_params.GetTypeAtOffset(i) == SCC_RAW_STRING_POINTER) {
strings[i] = stredup((const char *)(size_t)_global_string_params.GetParam(i));
dst[i] = (size_t)strings[i];
backup[i] = (const char *)(size_t)_global_string_params.GetParam(i);
} else {
strings[i] = nullptr;
dst[i] = _global_string_params.GetParam(i);
backup[i] = _global_string_params.GetParam(i);
}
}
}
Expand Down
7 changes: 4 additions & 3 deletions src/strings_func.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "string_type.h"
#include "gfx_type.h"
#include "core/bitmath_func.hpp"
#include "core/span_type.hpp"
#include "vehicle_type.h"

/**
Expand Down Expand Up @@ -85,9 +86,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) = delete; // block passing temporaries to SetDParamStr

void CopyInDParam(const uint64 *src, int num);
void CopyOutDParam(uint64 *dst, int num);
void CopyOutDParam(uint64 *dst, const char **strings, StringID string, int num);
void CopyInDParam(const span<const StringParameterBackup> backup);
void CopyOutDParam(std::vector<StringParameterBackup> &backup, size_t num);
void CopyOutDParam(std::vector<StringParameterBackup> &backup, size_t num, StringID string);

uint64_t GetDParam(size_t n);

Expand Down