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

23 Apr 2015


      On 22 April 2015 at 19:30, Matteo Bruni mbruni@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".

    

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