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
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
On Sun, 7 Dec 2014, Sebastian Lackner wrote:
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.
Ah, thanks for the pointer - I'll move the implementation there, then.
That will probably make the issues that patches 1-2 fixed (in wrc and makedep) go away, since the tests will be for a DLL without dashes in the name, and with a shorter name, but hopefully they can be picked up anyway.
// Martin
On Sun, 7 Dec 2014, Sebastian Lackner wrote:
- Include files should also be added as a separate patch. At the moment
the formatting looks a bit ugly (random number of linebreaks?).
I had some sort of grouping of the functions into groups with similar functions, but I agree that it looks mostly random.
- In the specfile use 'wstr' for arguments which are widechar strings.
This allows to print them with the +relay debug channel.
Ok - can this be used for strings that aren't null terminated as well (like the parameter to WindowsCreateString)?
- Not sure how strict Wine is about the coding style, but generally a
slighly different style is preferred for new code.
Ok, will change
For patch 4/4:
- C++-Style comments like "// ..." are not allowed in Wine source,
instead use "/* ... */" for single-line comments or
Ok, will change
- 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 ---
Ok, will change
- 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.
Yeah, although the checking here doesn't cost much extra effort either, as long as it's wrapped into a macro
- check_string should also show the line number because it is used very
often.
Indeed, that will be helpful.
- 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:
Ok, I'll see if I manage to get it split in a sensible way.
// Martin
On 07.12.2014 20:54, Martin Storsjö wrote:
- In the specfile use 'wstr' for arguments which are widechar
strings. This allows to print them with the +relay debug channel.
Ok - can this be used for strings that aren't null terminated as well (like the parameter to WindowsCreateString)?
No, for such strings you can keep using 'ptr'. Wine sets up an exception handler to catch invalid pointers or non-nullterminated strings, but the output will not really be useful in such a situation anyway.
Btw, if you're also going to implement stubs for combase (and my theory is right, that all the winrt related functions are exported from there), you could also move the api-ms-win-core-winrt implementation in a separate patch.
Regards, Sebastian
-
Martin Storsjö -
Sebastian Lackner