[PATCH v2 02/15] propsys: Implement several PropVariantInit*() functions

[GOUJON Alexandre]

Sorry for the late reply, I've been quite busy lately...

On 18/07/2015 18:26, Jonas Kümmerlin wrote:
> <snip>
>   
> +HRESULT WINAPI InitPropVariantFromCLSID(REFCLSID clsid, PROPVARIANT *ppropvar)
> +{
> +    TRACE("(%p %p)\n", clsid, ppropvar);
> +
> +    ppropvar->u.puuid = CoTaskMemAlloc(sizeof(CLSID));
I looked at current proposys/propvar.c and parameter check is inconsistent.
We sometimes check input pointer against NULL and return E_FAIL like in 
InitPropVariantFromGUIDAsString.
I guess it does not matter until an app depend on it but if you want to, 
you can add tests.

> <snip>
>   
> +#define DEFINE_INIT_FROM_VECTOR(ctype, functype, vtype, usuffix) \
> +    HRESULT WINAPI InitPropVariantFrom##functype##Vector(const ctype *pval, ULONG cEl, PROPVARIANT *ppropvar) \
> +    { \
> +        TRACE("(%p %u %p)\n", pval, cEl, ppropvar); \
> +        \
> +        ppropvar->u.ca##usuffix.pElems = CoTaskMemAlloc(cEl * sizeof(ctype)); \
> +        if(!ppropvar->u.ca##usuffix.pElems) \
> +            return E_OUTOFMEMORY; \
> +        \
> +        ppropvar->vt = VT_VECTOR|VT_##vtype; \
> +        ppropvar->u.ca##usuffix.cElems = cEl; \
> +        memcpy(ppropvar->u.ca##usuffix.pElems, pval, cEl * sizeof(ctype)); \
> +        return S_OK; \
> +    }
> +
> +DEFINE_INIT_FROM_VECTOR(SHORT, Int16, I2, i)
> +DEFINE_INIT_FROM_VECTOR(USHORT, UInt16, UI2, ui)
> +DEFINE_INIT_FROM_VECTOR(LONG, Int32, I4, l)
> +DEFINE_INIT_FROM_VECTOR(ULONG, UInt32, UI4, ul)
> +DEFINE_INIT_FROM_VECTOR(LONGLONG, Int64, I8, h)
> +DEFINE_INIT_FROM_VECTOR(ULONGLONG, UInt64, UI8, uh)
> +DEFINE_INIT_FROM_VECTOR(double, Double, R8, dbl)
> +DEFINE_INIT_FROM_VECTOR(FILETIME, FileTime, FILETIME, filetime)
> +
> +#undef DEFINE_INIT_FROM_VECTOR
I know AJ doesn't like macro-as-a-function and I think he's right.
Would it be possible to define a helper method, write plain functions 
calling it and play with something like FIELD_OFFSET ?
The scheme is aways the same : allocate, check error, set vt and set 
field. The remaining part vary from function to function.
I don't know if it'd be cleaner or even feasible.

> +
> +HRESULT WINAPI InitPropVariantFromBooleanVector(const BOOL *pbool, ULONG cEl, PROPVARIANT *ppropvar)
> +{
> +    ULONG i;
> +    TRACE("(%p %u %p)\n", pbool, cEl, ppropvar);
> +
> +    ppropvar->u.cabool.pElems = CoTaskMemAlloc(cEl * sizeof(VARIANT_BOOL));
Why not use your PROPVARIANT_U here ?
Note that we generally name it U (see commit 
9ecc71213fea7cf1656b249ebe2cfb7c7b51927f)

> +/* FIXME: Make this available only if the compiler supports the inline keyword */
inline is only a hint for the compiler and should't matter (except if 
you're playing/comparing with inline function address).