[PATCH] comctl32: Avoid an unneeded lstrlenW() call.

[Francois Gouget]

Signed-off-by: Francois Gouget <fgouget(a)free.fr>
---

It seems wasteful to compute the length of the string when all we care 
about is the first character. Plus the 'if (*str)' pattern is pretty 
common and used extensively in Wine already.

 dlls/comctl32/toolbar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dlls/comctl32/toolbar.c b/dlls/comctl32/toolbar.c
index 6a51ad7fc17..ec3b49890fd 100644
--- a/dlls/comctl32/toolbar.c
+++ b/dlls/comctl32/toolbar.c
@@ -2435,7 +2435,7 @@ TOOLBAR_CustomizeDialogProc(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
                     btnInfo->btn = nmtb.tbButton;
                     if (!(nmtb.tbButton.fsStyle & BTNS_SEP))
                     {
-                        if (lstrlenW(nmtb.pszText))
+                        if (*nmtb.pszText)
                             lstrcpyW(btnInfo->text, nmtb.pszText);
                         else if (nmtb.tbButton.iString >= 0 && 
                             nmtb.tbButton.iString < infoPtr->nNumStrings)
-- 
2.19.1

[Zhiyi Zhang]

On 10/22/18 9:16 AM, Francois Gouget wrote:
> Signed-off-by: Francois Gouget <fgouget(a)free.fr>
> ---
>
> It seems wasteful to compute the length of the string when all we care 
> about is the first character. Plus the 'if (*str)' pattern is pretty 
> common and used extensively in Wine already.
>
> dlls/comctl32/toolbar.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/dlls/comctl32/toolbar.c b/dlls/comctl32/toolbar.c
> index 6a51ad7fc17..ec3b49890fd 100644
> --- a/dlls/comctl32/toolbar.c
> +++ b/dlls/comctl32/toolbar.c
> @@ -2435,7 +2435,7 @@ TOOLBAR_CustomizeDialogProc(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
>                      btnInfo->btn = nmtb.tbButton;
>                      if (!(nmtb.tbButton.fsStyle & BTNS_SEP))
>                      {
> -                        if (lstrlenW(nmtb.pszText))
> +                        if (*nmtb.pszText)]
> There is a behavior change with this modification. lstrlenW won't crash even if applications somehow
> replaced pszText with null. So we might want to check that pszText is not null before deferencing it.
>                              lstrcpyW(btnInfo->text, nmtb.pszText);
>                          else if (nmtb.tbButton.iString >= 0 && 
>                              nmtb.tbButton.iString < infoPtr->nNumStrings)

[Dmitry Timoshkov]

Zhiyi Zhang <zzhang(a)codeweavers.com> wrote:
> > @@ -2435,7 +2435,7 @@ TOOLBAR_CustomizeDialogProc(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
> >                      btnInfo->btn = nmtb.tbButton;
> >                      if (!(nmtb.tbButton.fsStyle & BTNS_SEP))
> >                      {
> > -                        if (lstrlenW(nmtb.pszText))
> > +                        if (*nmtb.pszText)]
> > There is a behavior change with this modification. lstrlenW won't crash even if applications somehow
> > replaced pszText with null. So we might want to check that pszText is not null before deferencing it.

Have a look at include/winbase.h: lstr* APIs in Wine code are silently
replaced by inline versions without an exception handler.

-- 
Dmitry.

[Francois Gouget]

On Mon, 22 Oct 2018, Dmitry Timoshkov wrote:
> Zhiyi Zhang <zzhang(a)codeweavers.com> wrote:
> > > @@ -2435,7 +2435,7 @@ TOOLBAR_CustomizeDialogProc(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
> > >                      btnInfo->btn = nmtb.tbButton;
> > >                      if (!(nmtb.tbButton.fsStyle & BTNS_SEP))
> > >                      {
> > > -                        if (lstrlenW(nmtb.pszText))
> > > +                        if (*nmtb.pszText)]
> > > There is a behavior change with this modification. lstrlenW won't crash even if applications somehow
> > > replaced pszText with null. So we might want to check that pszText is not null before deferencing it.
>
> Have a look at include/winbase.h: lstr* APIs in Wine code are silently
> replaced by inline versions without an exception handler.

Right. In the patch I forgot to mention that I checked Wine's 
lstrlen{A,W}() implementation in winbase.h but I forgot to check what 
the MSDN says (or the kernel32/string.c implementation). Given the 
inconsistency that's all the more reason to avoid using lstrlen{A,W}() 
in such a case (or at all actually).

In this case nmtb.pszText is set to Buffer which is a WCHAR Buffer[256]. 
So it cannot be NULL and MSDN's lstrlenW() exception handler would not 
be needed anyway.


-- 
Francois Gouget <fgouget(a)free.fr>              http://fgouget.free.fr/
 Advice is what we ask for when we already know the answer but wish we didn't
                                 -- Eric Jong