[prev in list] [next in list] [prev in thread] [next in thread] 

List:       wine-devel
Subject:    Re: [PATCH 1/7] regedit: Use a helper for LoadString() and add debug information
From:       Dmitry Timoshkov <dmitry () baikal ! ru>
Date:       2017-01-30 1:01:59
Message-ID: 20170130090159.376b559f.dmitry () baikal ! ru
[Download RAW message or body]

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.

-- 
Dmitry.



[prev in list] [next in list] [prev in thread] [next in thread]