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".