[PATCH 15/15] api-ms-win-core-winrt-string: Implement a few HSTRING functions.

[Martin Storsjö]

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?

> * http://msdn.microsoft.com/en-us/library/br224632(v=vs.85).aspx mentions:
> """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