On 10/5/21 06:57, Giovanni Mascellani wrote:
Hi,
Il 04/10/21 19:03, Zebediah Figura ha scritto:
On 10/4/21 3:32 AM, Giovanni Mascellani wrote:
Semantically, a NULL buffer is considered equivalent to the empty string. The accessor vkd3d_string_buffer_data implements this equivalence.
The reason for this change is to avoid asserting the result of a memory allocation in vkd3d_string_buffer_init, while at the same time keeping it always successful.
Well, I guess this is one alternative to 216057. I don't dislike it.
Mmmh, I'm not really sure why you see it as such, but it was not meant to. It's true that it conflicts with it (sorry for that, when I wrote and submitted it I hadn't yet seen your patch set), but the intention is another, namely remove the assert(buffer->buffer) in Matteo's patch. This has nothing to do with how to store type names, which you address in your patch.
Well, in the sense that 216057 (well, 216055 really) is mainly just there to get rid of the boilerplate around calling hlsl_type_to_string(). This patch does lesson that boilerplate a bit.
@@ -855,7 +856,7 @@ struct vkd3d_string_buffer *hlsl_type_to_string(struct hlsl_ctx *ctx, const stru if ((inner_string = hlsl_type_to_string(ctx, t))) { - vkd3d_string_buffer_printf(string, "%s", inner_string->buffer); + vkd3d_string_buffer_printf(string, "%s", vkd3d_string_buffer_data(inner_string)); hlsl_release_string_buffer(ctx, inner_string); }
This means that here, and everywhere else, you can get rid of the NULL check.
I'm pretty sure that in many cases vkd3d_string_buffer_data could be avoided, when it can be shown that the buffer cannot be NULL. Though ideally I'd like to encapsulate as much as possible the fact that the buffer can be NULL inside the accessor vkd3d_string_buffer_data, so I am using the accessor nearly everywhere (a couple of places were actually using knowledge of the internals of vkd3d_string_buffer, and I didn't change that, because after all we're writing C and nobody really expects full encapsulation).
I guess you're looking at this with the perspective that it's just there to avoid asserting the result of an allocation. That's fine, but I think that it's useful to avoid checking the result of one as well. Sort of like put_u32() and friends.
diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index ef9eaef5..8a2dd0e3 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -25,11 +25,9 @@ VKD3D_DEBUG_ENV_NAME("VKD3D_SHADER_DEBUG"); void vkd3d_string_buffer_init(struct vkd3d_string_buffer *buffer) { - buffer->buffer_size = 16; + buffer->buffer_size = 0; buffer->content_size = 0; - buffer->buffer = vkd3d_malloc(buffer->buffer_size); - assert(buffer->buffer); - memset(buffer->buffer, 0, buffer->buffer_size); + buffer->buffer = NULL; }
I'm assuming this was accidental.
That was actually the point of the whole patch.
Oh, I see now. Right, I that the whole point of preallocating the buffer was so that it was never NULL.
void vkd3d_string_buffer_cleanup(struct vkd3d_string_buffer *buffer) @@ -37,20 +35,29 @@ void vkd3d_string_buffer_cleanup(struct vkd3d_string_buffer *buffer) vkd3d_free(buffer->buffer); } +const char *vkd3d_string_buffer_data(const struct vkd3d_string_buffer *buffer) +{ + return buffer->buffer ? buffer->buffer : ""; +}
Should this be defined in vkd3d_shader_private.h as an inline function?
Good point about that.
Thanks, Giovanni.