[PATCH 6/6] wined3d: Introduce a scratch string buffers framework.

[Henri Verbeet]

On 22 April 2015 at 19:30, Matteo Bruni <mbruni at codeweavers.com> wrote:
> It's somewhat of an RFC at this point. In particular I'd like some
> feedback about the API.
In general, I think it goes in the right direction.

> An obvious variant would be to store the pointer to the parent
> wined3d_shader_buffer structure right before the string and just
> return the string itself (thus avoiding having to add ->buffer to
> all the uses of the string).
Personally, I don't particularly mind the couple of extra characters,
and it seems like a good thing to explicitly distinguish between
string buffers and regular strings.

> +        tex_reg_name = shader_sprintf_unsafe(&priv->string_buffers, "tex%u", stage);
>          shader_glsl_color_correction_ext(buffer, tex_reg_name, WINED3DSP_WRITEMASK_ALL,
>                  settings->op[stage].color_fixup);
The problem with this construction is that it is likely to break as
soon as shader_glsl_color_correction_ext() does anything that ends up
calling shader_get_buffer(). To make it worse,
shader_glsl_color_correction_ext() isn't aware of this because it just
gets a const char *, and shader_glsl_generate_ffp_fragment_shader()
isn't supposed to care about the internals of
shader_glsl_color_correction_ext().

> @@ -276,13 +276,17 @@ int shader_vaddline(struct wined3d_shader_buffer *buffer, const char *format, va
>      char *base = buffer->buffer + buffer->content_size;
>      int rc;
>      char *new_buffer;
> +    unsigned int new_buffer_size;
>
>      while(1)
>      {
>          rc = vsnprintf(base, buffer->buffer_size - buffer->content_size, format, args);
>          if (rc < 0 /* C89 */ || (unsigned int)rc >= buffer->buffer_size - buffer->content_size /* C99 */)
>          {
> -            new_buffer = HeapReAlloc(GetProcessHeap(), 0, buffer->buffer, buffer->buffer_size * 2);
> +            new_buffer_size = buffer->buffer_size * 2;
> +            while (rc > 0 && (unsigned int)rc >= new_buffer_size - buffer->content_size)
> +                new_buffer_size *= 2;
> +            new_buffer = HeapReAlloc(GetProcessHeap(), 0, buffer->buffer, new_buffer_size);
>              if (!new_buffer)
>              {
>                  ERR("The buffer allocated for the shader program string is too small at %d bytes.\n", buffer->buffer_size);
> @@ -290,7 +294,7 @@ int shader_vaddline(struct wined3d_shader_buffer *buffer, const char *format, va
>                  return -1;
>              }
>              buffer->buffer = new_buffer;
> -            buffer->buffer_size = buffer->buffer_size * 2;
> +            buffer->buffer_size = new_buffer_size;
>              base = buffer->buffer + buffer->content_size;
>          }
>          else
This is a bit of an unrelated change, but probably the right thing to
do. Note that I have an unsent patch that separates tracing the shader
source from shader_vaddline() though, so it's going to look a bit
different fairly soon.

> +static char shader_empty_string[] = "";
This should be const, right?

> +static void shader_sprintf_va_list(struct wined3d_shader_buffer *buffer, const char *format, va_list args)
I probably would have called that vprintf/vsprintf instead of sprintf_va_list.

> +{
> +    int ret;
> +    char *new_buffer;
> +    unsigned int new_buffer_size;
> +
> +    if (!buffer)
> +        return;
> +    while (1)
> +    {
> +        ret = vsnprintf(buffer->buffer, buffer->buffer_size, format, args);
> +        if (ret < 0 || (unsigned int)ret >= buffer->buffer_size)
> +        {
> +            new_buffer_size = buffer->buffer_size * 2;
> +            while (ret > 0 && (unsigned int)ret >= new_buffer_size)
> +                new_buffer_size *= 2;
> +            new_buffer = HeapReAlloc(GetProcessHeap(), 0, buffer->buffer, new_buffer_size);
> +            if (!new_buffer)
> +            {
> +                ERR("Couldn't allocate buffer for temporary string.\n");
> +                buffer->buffer[0] = 0;
> +                return;
> +            }
> +            buffer->buffer = new_buffer;
> +            buffer->buffer_size = new_buffer_size;
> +        }
> +        else
> +        {
> +            break;
> +        }
> +    }
> +}
Once tracing the shader source is removed from shader_vaddline(), this
comes down to a shader_buffer_clear() followed by shader_vaddline().
And arguably shader_get_buffer() should always shader_buffer_clear()
the buffers it returns.

> +/* This function immediately puts the buffer back into the free list (it's
> + * intended to be used when the string is to be used and discarded immediately,
> + * mostly useful to avoid having to move the string pointer out of the block). */
> +const char *shader_sprintf_unsafe(struct list *buffer_list, const char *format, ...)
Not a fan, for the reasons mentioned at
shader_glsl_generate_ffp_fragment_shader() above.

> +void shader_release_buffer(struct list *list, struct wined3d_shader_buffer *buffer)
> +{
> +    if (!buffer)
> +        return;
Can buffer ever legitimately be NULL?

> +void shader_buffer_list_free(struct list *list)
> +{
> +    struct wined3d_shader_buffer *buffer, *buffer_next;
> +
> +    LIST_FOR_EACH_ENTRY_SAFE(buffer, buffer_next, list, struct wined3d_shader_buffer, entry)
> +    {
> +        shader_buffer_free(buffer);
> +        HeapFree(GetProcessHeap(), 0, buffer);
> +    }
> +}
> +
This doesn't actually remove the buffers from the list. There may also
be an argument for wrapping the list itself in a structure, similar to
struct wined3d_private_store.

>  struct wined3d_shader_buffer
>  {
At some point we'll probably want to rename this to something like
"wined3d_string_buffer".