RE: shlwapi 405/406

[Robert Shearman]

Hi,

Just a few comments on an otherwise good patch:
> -static BOOL URL_JustLocation(LPCWSTR str)
> +static BOOL WINAPI URL_JustLocation(LPCWSTR str)
> ...
> -static LPCWSTR  URL_ScanID(LPCWSTR start, LPDWORD size,
> WINE_URL_SCAN_TYPE type)
> +static LPCWSTR WINAPI URL_ScanID(LPCWSTR start, LPDWORD size,
> WINE_URL_SCAN_TYPE type)

Is there any reason for changing the calling convention of these two
functions?
> +/****************************************************************
> *********
> + *  MLBuildResURLA	[SHLWAPI.406]
> + *
> + * See MLBuildResURLA.
> + */
> +HRESULT WINAPI MLBuildResURLW(LPCWSTR lpszLibName, HMODULE hMod,
> DWORD dwFlags,
> +                              LPCWSTR lpszRes, LPWSTR lpszDest,
> DWORD dwDestLen)
> +{
> +  static const WCHAR szRes[] = { 'r','e','s',':','/','/','\0' };
> +#define szResLen (sizeof(szRes)/sizeof(WCHAR) - sizeof(WCHAR))

Should this not be #define szResLen (sizeof(szRes)/sizeof(WCHAR) - 1) ?
> +  HRESULT hRet = E_FAIL;
> +
> +  TRACE("(%s,%p,0x%08lx,%s,%p,%ld)\n", debugstr_w(lpszLibName),
> hMod, dwFlags,
> +        debugstr_w(lpszRes), lpszDest, dwDestLen);
> +
> +  if (!lpszLibName || !hMod || hMod == INVALID_HANDLE_VALUE ||
> !lpszRes ||
> +      !lpszDest || (dwFlags && dwFlags != 2))
> +    return E_INVALIDARG;
> +
> +  if (dwDestLen >= szResLen + 1)
> +  {
> +    dwDestLen -= (szResLen + 1);
> +    memcpy(lpszDest, szRes, sizeof(lpszDest));
> +
> +    hMod = MLLoadLibraryW(lpszLibName, hMod, dwFlags);
> +
> +    if (hMod)
> +    {
> +      WCHAR szBuff[MAX_PATH];
> +
> +      if (GetModuleFileNameW(hMod, szBuff, sizeof(szBuff)/sizeof(WCHAR)))
> +      {
> +        DWORD dwPathLen = strlenW(szBuff) + 1;
> +
> +        if (dwDestLen >= dwPathLen)
> +        {
> +          DWORD dwResLen;
> +
> +          dwDestLen -= dwPathLen;
> +          memcpy(lpszDest + szResLen, szBuff, dwPathLen * sizeof(WCHAR));
> +
> +          dwResLen = strlenW(lpszRes) + 1;
> +          if (dwDestLen >= dwResLen + 1)
> +          {
> +            lpszDest[szResLen + dwPathLen + dwResLen] = '/';
> +            memcpy(lpszDest + szResLen + dwPathLen, lpszRes,
> dwResLen * sizeof(WCHAR));
> +            hRet = S_OK;
> +          }
> +        }
> +      }
> +      MLFreeLibrary(hMod);
> +    }
> +  }
> +  return hRet;
> +}

Rob

shlwapi 405/406

[Jon Griffiths]

Hiya,
> Is there any reason for changing the calling convention of these
> two functions?

Yes, they are called more than once in the file, so stdcall produces
smaller code (no need to duplicate the argument stack pops).
Similarly any function thats only called once can be explicitly
inlined for smaller code.
> Should this not be #define szResLen (sizeof(szRes)/sizeof(WCHAR) -
> 1) ?

If you want to use -1, I think that should then be 

((sizeof(szRes)-1)/sizeof(WCHAR))

Since WCHARS are 2 bytes, not 1. The existing calculation looks right
to me, am I missing something?

Cheers,
Jon


=====
"Don't wait for the seas to part, or messiahs to come;
 Don't you sit around and waste this chance..." - Live

jon_p_griffiths(a)yahoo.com

__________________________________
Do you Yahoo!?
Yahoo! SiteBuilder - Free, easy-to-use web site design software
http://sitebuilder.yahoo.com

shlwapi 405/406

[Jon Griffiths]

Hiya,


> Should this not be #define szResLen (sizeof(szRes)/sizeof(WCHAR) -
1) ?

Sorry, I must have dozed off. You are of course right, I'll resend in
a minute. Good spotting!

Jon



=====
"Don't wait for the seas to part, or messiahs to come;
 Don't you sit around and waste this chance..." - Live

jon_p_griffiths(a)yahoo.com

__________________________________
Do you Yahoo!?
Yahoo! SiteBuilder - Free, easy-to-use web site design software
http://sitebuilder.yahoo.com

shlwapi 405/406

[Alexandre Julliard]

Jon Griffiths <jon_p_griffiths(a)yahoo.com> writes:
> Yes, they are called more than once in the file, so stdcall produces
> smaller code (no need to duplicate the argument stack pops).

Please don't do that. Internal functions should use standard C calling
conventions. We don't care about two bytes of extra code.

-- 
Alexandre Julliard
julliard(a)winehq.com

shlwapi 405/406

[Jon Griffiths]

Hi,
> Please don't do that. Internal functions should use standard C
> calling conventions. We don't care about two bytes of extra code.

Can do. Is there any reason why not though? After all, the entire
Win32 api does it...

Cheers,
Jon



=====
"Don't wait for the seas to part, or messiahs to come;
 Don't you sit around and waste this chance..." - Live

jon_p_griffiths(a)yahoo.com

__________________________________
Do you Yahoo!?
The New Yahoo! Shopping - with improved product search
http://shopping.yahoo.com

shlwapi 405/406

[Alexandre Julliard]

Jon Griffiths <jon_p_griffiths(a)yahoo.com> writes:
> > Please don't do that. Internal functions should use standard C
> > calling conventions. We don't care about two bytes of extra code.
>
> Can do. Is there any reason why not though? After all, the entire
> Win32 api does it...

Precisely, this way it's obvious which functions are meant to be used
by Win32 apps and which are internal to Wine.

-- 
Alexandre Julliard
julliard(a)winehq.com

shlwapi 405/406

[Jon Griffiths]

Hi,

--- Alexandre Julliard <julliard(a)winehq.com> wrote:
> Precisely, this way it's obvious which functions are meant to be
> used by Win32 apps and which are internal to Wine.

Hmm, I think any programmer who thinks a static function in one of
the implementation files of a given dll can possibly be part of the
exported Win32 API has larger problems than just confusion over what
the API covers ... and I believe the documentation sitting right
above the protoypes of exported functions should suffice to
distinguish for those who want to learn Win32 programming by scanning
the Wine source tree.

But since you're adament, this should probably be made a janitorial
task; internal stdcall functions are currently used in other places
(5 other files in shlwapi, for example). I'll resend.

Cheers,
Jon


=====
"Don't wait for the seas to part, or messiahs to come;
 Don't you sit around and waste this chance..." - Live

jon_p_griffiths(a)yahoo.com

__________________________________
Do you Yahoo!?
The New Yahoo! Shopping - with improved product search
http://shopping.yahoo.com

shlwapi 405/406

[Dimitrie O. Paun]

On September 29, 2003 05:31 pm, Jon Griffiths wrote:
> But since you're adament, this should probably be made a janitorial
> task; internal stdcall functions are currently used in other places
> (5 other files in shlwapi, for example).

It doesn't seem that much of a problem, a patch would be simpler
than to introduce a new janitorial task... :)

-- 
Dimi.

shlwapi 405/406

[Alexandre Julliard]

Jon Griffiths <jon_p_griffiths(a)yahoo.com> writes:
> Hmm, I think any programmer who thinks a static function in one of
> the implementation files of a given dll can possibly be part of the
> exported Win32 API has larger problems than just confusion over what
> the API covers ...

Static functions can definitely be part of the API. The best example
is function pointers in COM interfaces.

-- 
Alexandre Julliard
julliard(a)winehq.com