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
Conversation
b3571db
to
6d3f2c9
Compare
I could be slightly worried that introducing the variant type could be a performance problem, depending on how often it would be used. Some strings are certainly queried a lot. Another thought: Combine the string id with its parameter vector, as well as an (optional) cached formatted version of the string, and the language the string was formatted with (for invalidating the cache). It would use more memory, but could avoid a number of string formatting calls. |
I wouldn't know; it's only for cases where an actual backup of the string parameters is made, that is restored at some later point (tooltip, news, errors). Would having a struct with
I'm not sure whether that's going to work nicely when there are multiple format calls with different string ids with the same parameter vector. See for example the news code. |
So essentially the question is: is master...rubidium42:string-backup-struct better or worse than this PR? |
6d3f2c9
to
11875c6
Compare
Changed this over to the |
/* 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Motivation / Problem
Use of two or three parameters to retain the backup, and as such making it harder to generalize.
Furthermore the use of
stredup
and the associated manual memory management.Description
Add StringParameterBackup struct that contains either a
uint64_t
orstd::string
, astd::vector
for it and the needed functions for it to create a backup or restore it.Convert the different users of the parameter backups to the new type/functions.
Remove the old functions.
Limitations
None that I can think of, except a bit more memory usage. But in the end, when next PRs get merged safer and simpler code differentiating between strings and integers.
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.