[PATCH vkd3d 1/3] vkd3d-shader/hlsl: Set the name for anonymous structures at creation time.

[Giovanni Mascellani]

Hi,

Il 04/10/21 03:51, Zebediah Figura ha scritto:
> This series is kind of an RFC, though, because I'm not actually sure we want to
> do this. The obvious benefit is that we get rid of a lot of common boilerplate
> that's otherwise pretty hard to sidestep—as shown by the diffstat in patch 3. On
> the other hand, we might want to do something like GCC's "x <aka y>", in which
> case this would be a step in the wrong direction (and a pretty impactful step at
> that.)

I don't have strong feelings about this series, neither for nor against. 
On one hand, negative diffstats are (at feature parity, and sometimes 
even without it) very welcome. On the other hand, the removed code is 
basically due the necessity of having to explicitly name intermediate 
values because then you have to call their destructors. This is 
something I more or less give up about then working with C, so I see 
less value in removing this single instance.

Another alternative would be to be as we (more or less implicitly) we 
currently do for IR nodes: leave everything that you allocate in some 
container, so that it can be destroyed along with the context. Just loud 
thinking, though, I am not sure it's a smart way to go either.

> (This series also has us taking a bit more memory, but it's not a lot. We
> already allocate names for all numeric types, since they aren't keywords.)

Agree that's not a real concern.

> --- a/libs/vkd3d-shader/hlsl.y
> +++ b/libs/vkd3d-shader/hlsl.y
> @@ -2075,7 +2075,7 @@ struct_declaration:
>   
>               if (!$3)
>               {
> -                if (!$2->name)
> +                if (!strcmp($2->name, "<unnamed>"))
>                       hlsl_error(ctx, @2, VKD3D_SHADER_ERROR_HLSL_INVALID_SYNTAX,
>                               "Anonymous struct type must declare a variable.");
>                   if (modifiers)

Personally I don't like using user-facing values for business decisions. 
If you want to mark a struct as unnamed, then you should use a bool 
variable for that, not relying on a somewhat arbitrary name.

Granted, it would work, but I don't like the code smell (which is 
arguably present in the old code as well, though).

If you really wanted to mark types as unnamed by storing a well-known 
name, at least I'd ask that this well-known name is represented by a 
named constant.

Thanks, Giovanni.