Hi Martin,
a few things where I am not sure about:
[PATCH 05/10]
[...]
+@ stub COMBASE_63 +@ stub COMBASE_64 +@ stub COMBASE_65 +@ stub COMBASE_66 +@ stub COMBASE_67 +@ stub COMBASE_68
[...]
Are these really named exports, or exports by ordinal? If they are by ordinal and have no name assigned they are probably internal, and its not worth exporting them in Wine. I also noticed that combase.dll exports a lot of functions (Co*), which in Wine are implemented in ole32. It probably makes sense to sync that, but that can also be added in a later patch in my opinion. I don't think make_specfiles already supports such a situation.
[PATCH 06/10 and also following patches]
+/***********************************************************************
RoInitialize (api-ms-win-core-winrt-l1-1-0.@)- */
Those comments should probably be changed.
[PATCH 07/10]
Does creating an empty string really return a HSTRING or just NULL (which seems to behave identical)? It probably makes sense to add a test for that.
Does WindowsCreateString or WindowsCreateStringReference strip terminating NULL chars? (Embedded NULL chars are allowed obviously, but it might be possible that trailing NULL chars are handled differently).
nsivov on #winehackers also suggested that it might be worth trying to find out what the structure is on Windows, but not sure if its worth the effort. If we're not trying at all to be compatible, we could get rid of one of the two memory allocations by allocating (sizeof(hstring_private) + length) as a single chunk, and setting ->buffer according.
[PATCH 09/10]
I would also suggest adding tests for the content of the buffer. You can use for example memcmp(...) to verify that the content is correct.
[PATCH 10/10]
Is the buffer initialized? Are terminating NULLs stripped from the buffer?
Regards, Sebastian
Hi Sebastian,
On Mon, 8 Dec 2014, Sebastian Lackner wrote:
Hi Martin,
a few things where I am not sure about:
[PATCH 05/10]
[...]
+@ stub COMBASE_63 +@ stub COMBASE_64 +@ stub COMBASE_65 +@ stub COMBASE_66 +@ stub COMBASE_67 +@ stub COMBASE_68
[...]
Are these really named exports, or exports by ordinal? If they are by ordinal and have no name assigned they are probably internal, and its not worth exporting them in Wine.
Ah, indeed, they are probably just exports by ordinal (the numbers match the ordinals).
There's a bunch of other functions with numbers, like ObjectStublessClient3 (3-32), NdrProxyForwardingFunction3 (3-32) as well - should I skip those as well, or is there any point in keeping them?
I also noticed that combase.dll exports a lot of functions (Co*), which in Wine are implemented in ole32. It probably makes sense to sync that, but that can also be added in a later patch in my opinion. I don't think make_specfiles already supports such a situation.
Ok, I can try to sync those as well.
[PATCH 06/10 and also following patches]
+/***********************************************************************
RoInitialize (api-ms-win-core-winrt-l1-1-0.@)- */
Those comments should probably be changed.
Ah, of course.
[PATCH 07/10]
Does creating an empty string really return a HSTRING or just NULL (which seems to behave identical)? It probably makes sense to add a test for that.
Hmm, indeed, it does return NULL. I'll add a test for that as well.
Does WindowsCreateString or WindowsCreateStringReference strip terminating NULL chars? (Embedded NULL chars are allowed obviously, but it might be possible that trailing NULL chars are handled differently).
No, it doesn't seem to do anything special with those. Will add tests for that as well.
nsivov on #winehackers also suggested that it might be worth trying to find out what the structure is on Windows, but not sure if its worth the effort. If we're not trying at all to be compatible, we could get rid of one of the two memory allocations by allocating (sizeof(hstring_private)
- length) as a single chunk, and setting ->buffer according.
Ok, will do.
I haven't tried to figure out the internal structure on windows, since the API (at least so far, and at least as long as there's no more of Windows Runtime involved) seems to suggest it's only an internal detail.
[PATCH 09/10]
I would also suggest adding tests for the content of the buffer. You can use for example memcmp(...) to verify that the content is correct.
Ok, will do.
[PATCH 10/10]
Is the buffer initialized? Are terminating NULLs stripped from the buffer?
No, the buffer seems to be uninitialized. Terminating NULLs aren't stripped but treated as embedded NULLs just like in the other functions.
// Martin
Hi Martin,
On 08.12.2014 09:14, Martin Storsjö wrote:
Are these really named exports, or exports by ordinal? If they are by ordinal and have no name assigned they are probably internal, and its not worth exporting them in Wine.
Ah, indeed, they are probably just exports by ordinal (the numbers match the ordinals).
There's a bunch of other functions with numbers, like ObjectStublessClient3 (3-32), NdrProxyForwardingFunction3 (3-32) as well - should I skip those as well, or is there any point in keeping them?
Those are really exports by name, but on Windows they are also exported by ole32 and missing in the current Wine specfile. Since this is most likely also an internal implementation detail (which is hopefully never directly used) it should be okay to skip them, in my opinion.
I also noticed that combase.dll exports a lot of functions (Co*), which in Wine are implemented in ole32. It probably makes sense to sync that, but that can also be added in a later patch in my opinion. I don't think make_specfiles already supports such a situation.
Ok, I can try to sync those as well.
If you find a way to do that it would be nice, but not sure if its possible with the existing make_specfiles script.
nsivov on #winehackers also suggested that it might be worth trying to find out what the structure is on Windows, but not sure if its worth the effort. If we're not trying at all to be compatible, we could get rid of one of the two memory allocations by allocating (sizeof(hstring_private) + length) as a single chunk, and setting ->buffer according.
Ok, will do.
I guess you already noticed it, but it should be (sizeof(hstring_private) + length + 1) of course. ;)
I haven't tried to figure out the internal structure on windows, since the API (at least so far, and at least as long as there's no more of Windows Runtime involved) seems to suggest it's only an internal detail.
Well, I guess this idea is mainly inspired by msvcrt internal structures. Although its not officially documented a lot of applications mess around with the internal datastructures. Nevertheless, since we are not aware of any application doing that, its probably not worth the effort. I guess our current implementation is already very close to what MS does, so it could be easily fixed if it turns out to be necessary later.
Regards, Sebastian
-
Martin Storsjö -
Sebastian Lackner