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

[Sebastian Lackner]

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