From: Matteo Bruni <matteo.mystral@gmail.com>
Subject: Re: [PATCH vkd3d 1/6] vkd3d-shader: Return a vkd3d_string_buffer from hlsl_type_to_string().
Message-Id: <CABvNrtOyE9M07q+FTcmqU0FkLE5R8dFm_JCVS48TGtucvdtenQ@mail.gmail.com>
Date: Fri, 26 Feb 2021 19:08:53 +0100
In-Reply-To: <20210223215701.477435-1-zfigura@codeweavers.com>
References: <20210223215701.477435-1-zfigura@codeweavers.com>

On Tue, Feb 23, 2021 at 10:57 PM Zebediah Figura
<zfigura@codeweavers.com> wrote:
>
> Signed-off-by: Zebediah Figura <zfigura@codeweavers.com>
> ---
>  libs/vkd3d-shader/hlsl.c                 | 62 +++++++++++++-----------
>  libs/vkd3d-shader/hlsl.h                 |  4 +-
>  libs/vkd3d-shader/hlsl.y                 | 20 +++++---
>  libs/vkd3d-shader/vkd3d_shader_main.c    | 54 +++++++++++++++++++++
>  libs/vkd3d-shader/vkd3d_shader_private.h | 11 +++++
>  5 files changed, 114 insertions(+), 37 deletions(-)
>
> diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
> index 52b3dd10..78eacf28 100644
> --- a/libs/vkd3d-shader/hlsl.c
> +++ b/libs/vkd3d-shader/hlsl.c

>  const char *debug_hlsl_type(const struct hlsl_type *type)
>  {
> +    struct vkd3d_string_buffer_list string_buffers;
> +    struct vkd3d_string_buffer *string;
>      const char *ret;
> -    char *string;
>
> -    if (!(string = hlsl_type_to_string(type)))
> +    vkd3d_string_buffer_list_init(&string_buffers);
> +    if (!(string = hlsl_type_to_string(&string_buffers, type)))
>          return NULL;
> -    ret = vkd3d_dbg_sprintf("%s", string);
> -    vkd3d_free(string);
> +    ret = vkd3d_dbg_sprintf("%s", string->buffer);
> +    vkd3d_string_buffer_release(&string_buffers, string);
> +    vkd3d_string_buffer_list_cleanup(&string_buffers);
>      return ret;
>  }

How painful would it be to pass the context (and thus string_buffers)
through to this function?

> diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h
> index e837dbcd..2ef3d5da 100644
> --- a/libs/vkd3d-shader/vkd3d_shader_private.h
> +++ b/libs/vkd3d-shader/vkd3d_shader_private.h
> @@ -865,17 +865,28 @@ bool shader_sm4_is_end(void *data, const DWORD **ptr) DECLSPEC_HIDDEN;
>
>  struct vkd3d_string_buffer
>  {
> +    struct list entry;
>      char *buffer;
>      unsigned int buffer_size;
>      unsigned int content_size;
>  };
>
> +struct vkd3d_string_buffer_list
> +{
> +    struct list list;
> +};

Since this is the second time we implement it and hindsight is usually
very good, I'd prefer it if we used an array + vkd3d_array_reserve()
to store the stack of available vkd3d_string_buffer structs.
Also it would be nice to rename struct vkd3d_string_buffer_list to
something that explains its function rather than what it is, but as
usual I don't have any good suggestions...