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

[Zebediah Figura (she/her)]

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