[PATCH 3/4] api-ms-win-core-winrt-string: Implement a few HSTRING functions.

Sebastian Lackner sebastian at fds-team.de
Sun Dec 7 10:31:12 CST 2014


In addition to my previous mail, I am also not really sure if adding these tests and implementation to api-ms-* dlls is a good idea. As you might know on Windows the implementation is slightly different, and the redirection of api-* dlls to their corresponding real DLL is done by a purely virtual table (PEB->ApiSetMap), see http://www.geoffchappell.com/studies/windows/win32/apisetschema/index.htm. The stub dlls are only needed to make compilers happy, but never really used. As far as I know the real implementation on Windows of these string functions (and also the rest of the Ro* api) is in combase.dll.

On 07.12.2014 17:18, Sebastian Lackner wrote:
> Hi Martin,
> 
> most of the patch looks okay, but I think there are a few things you
> should (or could) change.
> 
> 
> 
> For patch 3/4:
> 
> * I would split it up in several patches, and after adding the
> minimally necessary functions always combine the implementation with
> the corresponding tests. The huge number of functions in a single
> patch makes it difficult to review.
> 
> * Include files should also be added as a separate patch. At the
> moment the formatting looks a bit ugly (random number of
> linebreaks?). I would also suggest to rename the function arguments
> to make sure they don't look exactly like MSDN.
> 
> * Rename function arguments in the implementation itself, to avoid
> that they look identical to MSDN.
> 
> * Internal helper functions (like alloc_string) do not necessarily
> need to return a HRESULT return value, when there are only two
> possible results. I would suggest using BOOL instead.
> 
> * In the specfile use 'wstr' for arguments which are widechar
> strings. This allows to print them with the +relay debug channel.
> 
> * Not sure how strict Wine is about the coding style, but generally a
> slighly different style is preferred for new code.
> 
> Instead of
> 
> --- snip --- if (...) { .... } --- snip ---
> 
> the preferred version is
> 
> --- snip --- if (...) { ... } --- snip ---
> 
> The same applies also for for and do-while loops. Moreover cast
> operations like "(HSTRING)string" should usually have no space
> inbetween of the type and variable name.
> 
> 
> 
> For patch 4/4:
> 
> * C++-Style comments like "// ..." are not allowed in Wine source,
> instead use "/* ... */" for single-line comments or
> 
> --- snip --- /* ... * ... */ --- snip ---
> 
> for multi-line comments.
> 
> * To avoid crashes on machines where this library is not available,
> it is recommended to skip the tests in such a case. For example:
> 
> --- snip --- hmod = LoadLibraryA(...); if (!hmod) { win_skip("Failed
> to load api-ms-win-core-winrt-string-l1-1-0, skipping tests\n"); 
> return; } --- snip ---
> 
> * It is okay to use define macros for loading function pointers, but
> they should be defined directly before, and undefined immediately
> afterwards, to avoid collision with other definitions. I also don't
> think that checking for != NULL is really necessary, these api-* dlls
> have a well-defined set of export functions, so it is basically
> guaranteed that all functions are available.
> 
> * check_string should also show the line number because it is used
> very often. The general way to do that is:
> 
> --- snip --- #define check_string(str, length, has_null)
> _check_string(__LINE__, str, length, has_null) static void
> check_string(int line, HSTRING str, UINT32 length, BOOL has_null) { 
> ... ok_(__FILE__, line)(condition, ...); ... } --- snip ---
> 
> This allows to determine much easier where something goes wrong.
> 
> * I would also suggest to split the tests. There is no need to put
> everything in a single function, especially because api-*-string only
> implements string functions. ;) I would prefer something like:
> 
> --- snip --- START_TEST(...) { if (!init_functions()) return;
> 
> test_CreateString(); test_DuplicateString(); ... } --- snip ---
> 
> (Of course this doesn't mean that WindowsCreateString can only be
> used in one function.)
> 
> 
> Regards, Sebastian
> 




More information about the wine-devel mailing list