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