[PATCH vkd3d] vkd3d-shader: Allow a NULL buffer in vkd3d_string_buffer.

[Giovanni Mascellani]

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.

>> @@ -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).

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

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