Hi Sebastian,
On Mon, 1 Dec 2014, Sebastian Lackner wrote:
On 01.12.2014 11:27, Martin Storsjo wrote:
- buf = malloc(sizeof(*buf));
- if (!buf)
return E_OUTOFMEMORY;- buf->buffer = malloc((length + 1) * sizeof(*buf->buffer));
- if (!buf->buffer) {
free(buf);return E_OUTOFMEMORY;- }
Some of your previous patches also contain changes to ./configure which is an autogenerated file. Afaik its not allowed to include that when submitting patches.
Ok, I'll keep that in mind (and I can resubmit them without those changes if that's requested).
Feedback for this part:
- HSTRING and HSTRING_HEADER should probably be defined in some include
fine. To ensure that the size of private structure matches you could also use C_ASSERT(...), and maybe even disable align of the structure fields.
Right, they should probably go into hstring.h and winstring.h then. (One needs to take more care about the HSTRING_HEADER struct if making it public though, since the public definition should match the right size in the ABI and not have any public meaningful members, only placeholder to make it a certain size, while the internals of the DLL uses that to store certain fields.)
- You should avoid using malloc(...) in Wine source
Ok, I should have guessed that. Is it ok to use memcpy for copying data though?
"""Calling WindowsDeleteString decrements the reference count of the backing buffer, and if the reference count reaches 0, the Windows Runtime de-allocates the buffer."""
I don't see the concept of refcounted backing buffers anywhere in your code, which is most likely wrong. Its difficult to fix that later.
I had a really hard time understanding the MSDN part here - I don't find any corresponding function for increasing a refcount, so I don't see how the HSTRING itself would be refcounted. So I think that only matters once you pass those string handles into the full Windows Runtime itself.
I don't see how it would be hard to add that to the internal representation of HSTRING later though - as long as that representation is purely internal we should be able to change it. And if it needs to be synced with something else (e.g. Windows Runtime accessing data from within HSTRING directly without going through the functions here) we'd need to sync with some unofficial/unknown/unspecified ABI.
- I would suggest starting with some stub functions, and then add tests
before sending the actual implementation.
That sounds like a sensible approach, although it'll probably take some time before I've got time to do it that way. This patch is pretty much separate from the rest of this patchset anyway (and I've got much less concrete need for it, contrary to the other patches), so I don't mind if it's deferred/dropped for now, separate from the rest at least.
// Martin