On Wed Oct 26 05:39:19 2022 +0000, Zebediah Figura wrote:
Here too the first line doesn't tell much that is not sort of
obvious from the field declaration, and the others, while certainly useful, are a bit messy. I would avoid writing any comment for `type` and write something like this for `base_type`:
If `type` is `HLSL_CLASS_SCALAR`, `HLSL_CLASS_VECTOR` or
`HLSL_CLASS_MATRIX`, then `base_type` is one of `HLSL_TYPE_FLOAT`, `HLSL_TYPE_HALF`, `HLSL_TYPE_DOUBLE`, `HLSL_TYPE_INT`, `HLSL_TYPE_UINT` and `HLSL_TYPE_BOOL`. If `type` is `HLSL_CLASS_OBJECT`, then `base_type` is one of `HLSL_TYPE_SAMPLER`, `HLSL_TYPE_TEXTURE`, `HLSL_TYPE_UAV`, `HLSL_TYPE_PIXELSHADER`, `HLSL_TYPE_VERTEXSHADER`, `HLSL_TYPE_STRING` and `HLSL_TYPE_VOID`. In all the other cases `base_type` is not used. Maybe, although I think this is not exactly easier to read. Here's also where the messiness of hlsl_type kind of shows up. So, time to start a discussion: should we split up enum hlsl_base_type into numeric and object types, and use the existing union to distinguish the two? I.e. something like
struct hlsl_type { struct list entry; struct rb_entry scope_entry; enum hlsl_type_class type; const char *name; unsigned int modifiers; union { struct { enum hlsl_base_type base_type; unsigned int dimx, dimy; } numeric; struct { struct hlsl_struct_field *fields; size_t field_count; } record; struct { struct hlsl_type *type; unsigned int elements_count; } array; struct { enum hlsl_object_type object_type; enum hlsl_sampler_dim sampler_dim; struct hlsl_type *resource_format; } object; } e; };(I've moved a few more fields into the union while I'm at it.) The advantages of this, as I see it, are (a) self-documentation clarity, and (b) it lets us use fewer 'default' cases in things like dump_ir_constant(). [I like to avoid 'default' cases in general since they make it harder to notice when you've added a new enum value.] The disadvantage, of course, is churn. I don't think it'd make the code much more *complex*, though—the only place that really suffers is sm{1,4}_base_type(), but I'm okay with that.
Not directly related to your patches, but field declarations could be
fixed in the first place to make them more consistent. Right now we have an `hlsl_type` struct with fields `type` (of type `enum hlsl_type_class` with enum values beginning with `HLSL_CLASS_`) and `base_type` (of type `enum hlsl_base_type` with enum values beginning with `HLSL_TYPE_`). I would rename to `kind` (of type `enum hlsl_kind` with enum values beginning with `HLSL_KIND_`) and `base_type` (of type `enum hlsl_base_type` with enum values beginning with `HLSL_BASE_TYPE_`). But that's for another patch set. Despite the churn I'm inclined to agree with this. Although I don't see a need to rename "class" to "kind"; this isn't C++ after all ;-)
I'm totally in favor of Zeb's suggestion for `struct hlsl_type`. Only thing, I would use `Struct` instead of `record` (similarly to what we already to for `hlsl_ctx::builtin_types::Void`, but I can live with `record` too.
As for "class" vs "kind", I tend to prefer "kind" anyway because in type theory it is used to denote something along the lines of ["a type's type"](https://en.wikipedia.org/wiki/Kind_(type_theory)). But here too I can live with "class".