Hugh McMaster hugh.mcmaster@outlook.com wrote:
Probably using a flavour of lstrcpy that checks the target buffer length would be more appropriate. Although simply returning an empty buffer and an error indicator to the caller could be slightly better. Under which conditions LoadString may fail here?
The buffer size must be at least six WCHARs to allow for errorW, so we could just do something like if (!buf || count < 6) return;
In this code, LoadString() could fail if the resource ID doesn't exist or if count is 0 or 1. But since we are controlling all of the code, none of this is likely.
For MessageBox captions, if we pass in NULL, the default string "Error" is loaded by user32. So, apart from a few instances, we wouldn't have to handle captions at all. But we would have to do something if LoadString() failed for the body text, otherwise the user would see a blank message box.
The original code was: if (!LoadStringW(hInst, titleId, title, COUNT_OF(title))) lstrcpyW(title, errorW); if (!LoadStringW(hInst, resId, errfmt, COUNT_OF(errfmt))) lstrcpyW(errfmt, unknownW);
Do you think it is worth replacing? My main reason for adding load_string() was to avoid duplicating the FIXME line and to avoid "Unknown error string".
If LoadString should never fail under normal circumstances then perhaps simple assert(0) is enough, and there is no need to invent new code to handle errors that should never happen.