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

[Sebastian Lackner]

Hi Martin,

On 01.12.2014 12:40, Martin Storsjö wrote:
>> * 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.)

I know that this is a bit tricky. I would suggest to put the "dummy" version of HSTRING_HEADER into the include file, and then define a different structure inside of the implementation with C_ASSERTS(...) to ensure that the size between both matches. The code could then use something like this (similar to interfaces):

static inline struct hstring_private impl_from_HSTRING(HSTRING string)
{
    return (struct hstring_private *)string;
}

static inline struct hstring_private impl_from_HSTRING_HEADER(HSTRING_HEADER *header)
{
    return (struct hstring_private *)header;
}

...
{
  struct hstring_private *s1 = impl_from_HSTRING_HEADER(first_string);
  struct hstring_private *s2 = impl_from_HSTRING(another_string);
  ...

}

> 
>> * You should avoid using malloc(...) in Wine source
> 
> Ok, I should have guessed that. Is it ok to use memcpy for copying
> data though?

Yes, all other functions memcpy, memmove, ... are fine. For memory allocation you should always use the Heap functions (HeapAlloc, HeapFree, ...). Please note that calling HeapFree(...) with a null pointer is perfectly fine, so no additional if - checks are needed for that.

> 
>> * 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 think the main purpose of that is to implement fast versions for functions like SubString(...). If its not created with WindowsCreateStringReference(...) both strings can use the same backing buffer, which is faster than copying part of the string.

Normally strings are immutable, but as it is also possible to get a raw pointer to the buffer ( http://msdn.microsoft.com/en-us/library/br224636(v=vs.85).aspx ) an application might depend on this specific implementation detail. Tests would really help here to confirm my theory that changing the substring also modifies the original one.

> 
> 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.

Of course it is possible to change that, but when possible problems are known in advance it probably makes sense to think about that more carefully before including it. As already mentioned, tests could really be useful here to avoid later rewrites or issues in the implementation.

> 
>> * 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

Regards,
Sebastian