On 10/7/21 02:32, Giovanni Mascellani wrote:
Hi,
Il 06/10/21 16:54, Zebediah Figura (she/her) ha scritto:
Mmh, I am not sure I understand why that would lessen the boilerplate. You still need to name and destroy the string buffer, each of which takes a line. And vkd3d_string_buffer_data(string) is definitely longer than string->buffer. I don't really think this helps here.
Well, it lessens the boilerplate if you no longer need to check for success from hlsl_type_to_string().
In this form, you still have to check for the result of hlsl_type_to_string. My patch doesn't change the fact that hlsl_type_to_string can still return NULL if it cannot allocate the vkd3d_string_buffer as a whole. Notice that my patch deals with the buffer field of vkd3d_string_buffer being NULL, but the whole structure still needs to be correctly allocated.
Indeed, I misread it. Could we check the string_buffer itself for NULL as well, then, like you describe below? Maybe in a separate patch...
(en passant, I also notice that hlsl_type_to_string has insufficient error checking: it doesn't check the return value of vkd3d_string_buffer_prints, so if allocations fails there it will return an empty string; this doesn't lead to UB, but it generates a bad error message, which is still a bug, although much smaller)
I think that's inescapable; if we run out of memory we are going to do *something* wrong. Likely we can't even print the message in the first place.
I mean, it's very convenient to be able to not have to check for success from an allocation, in some way or another, if you're going to be doing that frequently. vkd3d_string_buffer_data() achieves that, so I think it's useful to see that as an end goal of vkd3d_string_buffer_data() in itself, and to take advantage of it to get rid of ubiquitous error checking.
Well, I guess this can be added as an additional goal, but we're not there yet. Taking this example of the error boilerplate:
struct vkd3d_string_buffer *string;
string = hlsl_type_to_string(ctx, $3); if (string) hlsl_error(ctx, @3, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE, "Matrix base type %s is not scalar.", vkd3d_string_buffer_data(string)); hlsl_release_string_buffer(ctx, string); YYABORT;
You cannot remove the "if (string)", because hlsl_type_to_string can still return NULL, and vkd3d_string_buffer_data wouldn't like it (no problems if string->buffer is NULL, but segfault if string itself is NULL).
It's true that vkd3d_string_buffer_data might be replaced with something like:
static inline const char *vkd3d_string_buffer_data(const struct vkd3d_string_buffer *buffer) { return buffer && buffer->buffer ? buffer->buffer : ""; }
In this case it would work as you say, though it would risk to generate a bad error message: "Matrix base type is not scalar.". As I said, less grave than UB, but still a bug. If you want to really achieve what you say, it seems to me that more hack^W engineering is required (and I am not against it).
Giovanni.