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