From: Józef Kucia <joseph.kucia@gmail.com>
Subject: Re: [PATCH 04/10] winevulkan: Implement global Vulkan function stubs and loading.
Message-Id: <CAKF0LCUAa8z3+ndunj5nn21rSEoeYsHpx1BcmEFHD3GvhhPZww@mail.gmail.com>
Date: Wed, 21 Feb 2018 10:51:28 +0100
In-Reply-To: <20180220062947.4192-5-thunderbird2k@gmail.com>
References: <20180220062947.4192-1-thunderbird2k@gmail.com> <20180220062947.4192-5-thunderbird2k@gmail.com>

On Tue, Feb 20, 2018 at 7:29 AM, Roderick Colenbrander
<thunderbird2k@gmail.com> wrote:
> Signed-off-by: Roderick Colenbrander <thunderbird2k@gmail.com>
> ---
>  dlls/winevulkan/vulkan.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 77 insertions(+), 2 deletions(-)
>
> diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c
> index 6d9aa3401f..7e505f4673 100644
> --- a/dlls/winevulkan/vulkan.c
> +++ b/dlls/winevulkan/vulkan.c
> @@ -34,12 +34,87 @@ WINE_DEFAULT_DEBUG_CHANNEL(vulkan);
>   */
>  #define WINE_VULKAN_ICD_VERSION 4
>
> -void * WINAPI wine_vk_icdGetInstanceProcAddr(VkInstance instance, const char *pName)
> +static VkResult WINAPI wine_vkCreateInstance(const VkInstanceCreateInfo *pCreateInfo, const VkAllocationCallbacks *pAllocator,
> +        VkInstance *pInstance);
> +static VkResult WINAPI wine_vkEnumerateInstanceExtensionProperties(const char *pLayerName, uint32_t *pPropertyCount,
> +        VkExtensionProperties *pProperties);
> +static PFN_vkVoidFunction WINAPI wine_vkGetInstanceProcAddr(VkInstance instance, const char *pName);

It could be better to move vk_global_dispatch_table and
wine_vk_get_global_proc_addr() to the bottom of the file. We would
need only one forward declaration for wine_vk_get_global_proc_addr().

> +
> +struct vulkan_func
> +{
> +    const char *name;
> +    void *func;
> +};
> +
> +const struct vulkan_func vk_global_dispatch_table[] = {

I think this should be "static const".

I personally would also prefer to put the opening bracket on a
separate line. It would be more consistent to always put curly
brackets on a separate line.

> +    {"vkCreateInstance", &wine_vkCreateInstance},
> +    {"vkEnumerateInstanceExtensionProperties", &wine_vkEnumerateInstanceExtensionProperties},
> +    {"vkGetInstanceProcAddr", &wine_vkGetInstanceProcAddr},
> +};
> +
> +static void *wine_vk_get_global_proc_addr(const char *name)
> +{
> +    int i;
> +    for (i = 0; i < sizeof(vk_global_dispatch_table) / sizeof(vk_global_dispatch_table[0]); i++)
> +    {
> +        if (strcmp(name, vk_global_dispatch_table[i].name) == 0)
> +        {
> +            TRACE("Found pName=%s in global table\n", name);
> +            return vk_global_dispatch_table[i].func;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static VkResult WINAPI wine_vkCreateInstance(const VkInstanceCreateInfo *pCreateInfo, const VkAllocationCallbacks *pAllocator,
> +        VkInstance *pInstance)
> +{
> +    FIXME("stub: %p %p %p\n", pCreateInfo, pAllocator, pInstance);
> +    return VK_ERROR_INCOMPATIBLE_DRIVER;
> +}
> +
> +static VkResult WINAPI wine_vkEnumerateInstanceExtensionProperties(const char *pLayerName, uint32_t *pPropertyCount,
> +        VkExtensionProperties *pProperties)
> +{
> +    FIXME("stub: %p %p %p\n", pLayerName, pPropertyCount, pProperties);
> +    return VK_ERROR_OUT_OF_HOST_MEMORY;
> +}
> +
> +static PFN_vkVoidFunction WINAPI wine_vkGetInstanceProcAddr(VkInstance instance, const char *pName)
>  {
> -    FIXME("stub: %p %s\n", instance, debugstr_a(pName));
> +    void *func;
> +    TRACE("%p %s\n", instance, debugstr_a(pName));
> +
> +    /* vkGetInstanceProcAddr can load most Vulkan functions when an instance is passed in, however
> +     * for a NULL instance it can only load global functions.
> +     */
> +    func = wine_vk_get_global_proc_addr(pName);

We may end up calling strcmp() with NULL. Generally, the Vulkan loader
should probably protect us from getting called with NULL. I still
think it would be better to return NULL, if !pname.