vkd3d-shader/hlsl: Add field-level documentation to the most important structs in hlsl.h.
I believe this information could be very useful for newcomers to the HLSL compiler.
Documentation often needs to reference other parts of the code, so it can be a problem to update these references if the referenced code is modified. However, I think that documentation in the data definitions (as opposed to documentation inside the functions) is easier to maintain, since each data type is declared in a single place, to which one often keeps going back for reference, so it is less probable to forget to update a comment.
Also, it opens up the possibility of explaining what things are, instead of how they are used, in some cases, avoiding these references to other places in the code.
I can think on the following additional benefits:
- Updates in these documentation comments, when the way a field is used changes, or when new fields are added, can help making future commits more readable.
- Some IDEs display the comments above the field/struct declaration when the cursor is over an use of this field/struct.
A hopefully not deceiving sketch:
Merge request reports
Activity
114 114 HLSL_ROW_MAJOR 115 115 }; 116 116 117 /* An HLSL data type. */ 117 118 struct hlsl_type 118 119 { 120 /* Linked list entry to store this hlsl_type in the context's linked list of hlsl_types. */ 119 121 struct list entry; - Comment on lines +120 to 121
From the field declaration it's already quite obvious that we are dealing with a linked list of objects of type
hlsl_type
, so I wouldn't repeat that piece of information. The only useful thing this I think should be mentioned here is whichstruct list
this field chains to, and everything else descends from that. So I would write something like "Chained with hlsl_ctx->types". Though I am not sure if "chained" is really the best possible word.The same mostly applies to all other
struct list
,struct rb_entry
andstruct rb_tree
. Thinking again, I wouldn't dislike a phrasing that distinguishes the container entry from the item entry (typing makes it already clear for RB trees, but not for lists). So maybe something like "Container entry in hlsl_ctx->types" here and "Item entry in hlsl_type->entry" in
hlsl_ctx
. The "direction" of the containment relationship is usually not hard to infer from the context, but if you're reading a struct for the first time sparing some mind cycles can be helpful.
115 115 }; 116 116 117 /* An HLSL data type. */ 117 118 struct hlsl_type 118 119 { 120 /* Linked list entry to store this hlsl_type in the context's linked list of hlsl_types. */ 119 121 struct list entry; 122 /* Search tree entry to store this hlsl_type in the context's search tree of types. The type's 123 * name is used as key. */ 120 124 struct rb_entry scope_entry; 125 /* These fields indicate to which category of hlsl_types this hlsl_type belongs. 126 * If type is HLSL_CLASS_OBJECT, base_type is used to indicate a subcategory of the type. 127 * If type is numeric, base_type is used to indicate the type of its components. 128 * base_type is not used when type is HLSL_CLASS_STRUCT or HLSL_CLASS_ARRAY. */ 121 129 enum hlsl_type_class type; 122 130 enum hlsl_base_type base_type; - Comment on lines +125 to 130
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 forbase_type
:If
type
isHLSL_CLASS_SCALAR
,HLSL_CLASS_VECTOR
orHLSL_CLASS_MATRIX
, thenbase_type
is one ofHLSL_TYPE_FLOAT
,HLSL_TYPE_HALF
,HLSL_TYPE_DOUBLE
,HLSL_TYPE_INT
,HLSL_TYPE_UINT
andHLSL_TYPE_BOOL
. Iftype
isHLSL_CLASS_OBJECT
, thenbase_type
is one ofHLSL_TYPE_SAMPLER
,HLSL_TYPE_TEXTURE
,HLSL_TYPE_UAV
,HLSL_TYPE_PIXELSHADER
,HLSL_TYPE_VERTEXSHADER
,HLSL_TYPE_STRING
andHLSL_TYPE_VOID
. In all the other casesbase_type
is not used.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 fieldstype
(of typeenum hlsl_type_class
with enum values beginning withHLSL_CLASS_
) andbase_type
(of typeenum hlsl_base_type
with enum values beginning withHLSL_TYPE_
). I would rename tokind
(of typeenum hlsl_kind
with enum values beginning withHLSL_KIND_
) andbase_type
(of typeenum hlsl_base_type
with enum values beginning withHLSL_BASE_TYPE_
). But that's for another patch set. 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 forbase_type
:If
type
isHLSL_CLASS_SCALAR
,HLSL_CLASS_VECTOR
orHLSL_CLASS_MATRIX
, thenbase_type
is one ofHLSL_TYPE_FLOAT
,HLSL_TYPE_HALF
,HLSL_TYPE_DOUBLE
,HLSL_TYPE_INT
,HLSL_TYPE_UINT
andHLSL_TYPE_BOOL
. Iftype
isHLSL_CLASS_OBJECT
, thenbase_type
is one ofHLSL_TYPE_SAMPLER
,HLSL_TYPE_TEXTURE
,HLSL_TYPE_UAV
,HLSL_TYPE_PIXELSHADER
,HLSL_TYPE_VERTEXSHADER
,HLSL_TYPE_STRING
andHLSL_TYPE_VOID
. In all the other casesbase_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 fieldstype
(of typeenum hlsl_type_class
with enum values beginning withHLSL_CLASS_
) andbase_type
(of typeenum hlsl_base_type
with enum values beginning withHLSL_TYPE_
). I would rename tokind
(of typeenum hlsl_kind
with enum values beginning withHLSL_KIND_
) andbase_type
(of typeenum hlsl_base_type
with enum values beginning withHLSL_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 useStruct
instead ofrecord
(similarly to what we already to forhlsl_ctx::builtin_types::Void
, but I can live withrecord
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". But here too I can live with "class".
120 /* Linked list entry to store this hlsl_type in the context's linked list of hlsl_types. */ 119 121 struct list entry; 122 /* Search tree entry to store this hlsl_type in the context's search tree of types. The type's 123 * name is used as key. */ 120 124 struct rb_entry scope_entry; 125 /* These fields indicate to which category of hlsl_types this hlsl_type belongs. 126 * If type is HLSL_CLASS_OBJECT, base_type is used to indicate a subcategory of the type. 127 * If type is numeric, base_type is used to indicate the type of its components. 128 * base_type is not used when type is HLSL_CLASS_STRUCT or HLSL_CLASS_ARRAY. */ 121 129 enum hlsl_type_class type; 122 130 enum hlsl_base_type base_type; 131 /* Sampling dimension of the hlsl_type, in case it is a resource type, e.g. a texture or 132 * sampler. */ 123 133 enum hlsl_sampler_dim sampler_dim; 134 /* String buffer with the name of the hlsl_type. */ 124 135 const char *name; Obvious and yet also incomplete. type->name isn't set for all types, only typedefs and structs.
As something of an aside that this just reminds me of: I wonder if we should precompute hlsl_type_to_string(), especially considering that in most cases it basically already is precomputed. It'd save us a bit of effort and OOM handling every time we print it to console.
125 137 unsigned int modifiers; 138 /* Size of the type values on each dimension; primarily intended for numeric types: scalars, 139 * vectors, and matrices.Other types may assign values to them for convenience. 140 * See type initialization functions. */ 126 141 unsigned int dimx; 127 142 unsigned int dimy; 143 128 144 union 129 145 { 146 /* Additional information if the type is HLSL_CLASS_STRUCT. */ 130 147 struct 131 148 { 149 /* Fields contained within the struct. */ 132 150 struct hlsl_struct_field *fields; 151 /* Number of fields contained in the struct. */ 133 152 size_t field_count; 146 /* Additional information if the type is HLSL_CLASS_STRUCT. */ 130 147 struct 131 148 { 149 /* Fields contained within the struct. */ 132 150 struct hlsl_struct_field *fields; 151 /* Number of fields contained in the struct. */ 133 152 size_t field_count; 134 153 } record; 154 /* Additional information if the type is HLSL_CLASS_ARRAY. */ 135 155 struct 136 156 { 157 /* Type of the array elements. */ 137 158 struct hlsl_type *type; 159 /* Number of elements contained within the array or HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT 160 * if it is unknown yet. */ 138 161 unsigned int elements_count; 117 118 struct hlsl_type 118 119 { 120 /* Linked list entry to store this hlsl_type in the context's linked list of hlsl_types. */ 119 121 struct list entry; 122 /* Search tree entry to store this hlsl_type in the context's search tree of types. The type's 123 * name is used as key. */ 120 124 struct rb_entry scope_entry; 125 /* These fields indicate to which category of hlsl_types this hlsl_type belongs. 126 * If type is HLSL_CLASS_OBJECT, base_type is used to indicate a subcategory of the type. 127 * If type is numeric, base_type is used to indicate the type of its components. 128 * base_type is not used when type is HLSL_CLASS_STRUCT or HLSL_CLASS_ARRAY. */ 121 129 enum hlsl_type_class type; 122 130 enum hlsl_base_type base_type; 131 /* Sampling dimension of the hlsl_type, in case it is a resource type, e.g. a texture or 132 * sampler. */ 123 133 enum hlsl_sampler_dim sampler_dim; - Comment on lines +131 to 133
In general I think that it's useful if comments like this spell out the precise rules, because there is good chance that that's what you'll look for if you read it. Something like:
If
base_type
isHLSL_TYPE_SAMPLER
then these are the allowed values: ...; ifbase_type
isHLSL_TYPE_TEXTURE
then these are the allowed values: ...; etc.
127 * If type is numeric, base_type is used to indicate the type of its components. 128 * base_type is not used when type is HLSL_CLASS_STRUCT or HLSL_CLASS_ARRAY. */ 121 129 enum hlsl_type_class type; 122 130 enum hlsl_base_type base_type; 131 /* Sampling dimension of the hlsl_type, in case it is a resource type, e.g. a texture or 132 * sampler. */ 123 133 enum hlsl_sampler_dim sampler_dim; 134 /* String buffer with the name of the hlsl_type. */ 124 135 const char *name; 136 /* Bitfield for storing type modifiers, subset of HLSL_TYPE_MODIFIERS_MASK */ 125 137 unsigned int modifiers; 138 /* Size of the type values on each dimension; primarily intended for numeric types: scalars, 139 * vectors, and matrices.Other types may assign values to them for convenience. 140 * See type initialization functions. */ 126 141 unsigned int dimx; 127 142 unsigned int dimy; Currently I think we set dimx/dimy for non-numeric types basically for the benefit of the sm1/sm4 backends, which is, y'know, the kind of thing that should be documented here. That said, I'm not sure I like that fact (see also my above proposal, which would make that the job of the backend).
In general I support the idea of documenting data structures rather than (or in addition to) code. But I think that your proposal still needs some work. I did a first pass on the first commit in order to outline what I think the philosophy of this kind of comments should be, so there can be some discussion.
Yeah, this patch is interesting. I'm not quite sure what to think of it. So some scattered thoughts below:
-
Fundamentally I think most (though not all) of these comments are obvious and unnecessary. But if they were that obvious, then we didn't need the comments in the first place, so I clearly need to adjust my expectations at least somewhat.
-
Looking at something like hlsl_type: intuitively I'd think it's pretty easy to guess what this is; it's a representation of a data type. It's also probably not hard to figure out what the contents of the type are just from the name, although some of them probably require an understanding of the HLSL language first.
-
More interesting are the questions that I think are not obvious just from looking at the source: is it source-level, or something further down? Is it unique? Where are all of the data types stored? [The answers, of course, being: yes, it's source-level; no, it's not unique; data types are stored in a list in hlsl_ctx.types].
-
There's also questions like: where are types constructed, and when are they retrieved? Same for individual fields—I can guess that the fields like base_type/dimx/dimy are set upon creation, but when is reg_size or bytecode_offset set and used?
-
Figuring out what kind of questions like this need to be asked is probably hard, though, and anticipating every question even harder. I'm basically trying to think of the kind of questions I've asked when I encounter a new project.
-
I think Giovanni kind of hit the nail on the head with one of the above comments—it's pretty obvious what sampler_dim means, the more interesting question is when is it set.
-
-
Then there's the awkward fact that a lot of the stuff that needs documentation should probably go away, like reg_size. (Coincidence?)
-
Some fields, like hlsl_ctx.profile, are not exactly clear just from the name and type what they are (because naming is hard and can only be so specific). But if you follow the chain to look at the contents of struct hlsl_profile_info, it becomes clearer. Is it still worth documenting hlsl_ctx.profile in that case? I don't know, maybe...
-
On the other hand, there's also stuff like hlsl_deref, which I think is a much less intuitive construction. I.e. I think it's clear what hlsl_type is, at least, but hlsl_deref is a bit more idiosyncratic. Same with hlsl_src (which is basically a node reference plus an entry in a def-use chain). So, y'know, not everything is obvious.
I'll probably set this aside for a few days and keep thinking about it, unless of course we end up having more discussion in that time ;-)
-
Finding the right balance is never easy, particularly when targetting an audience with a potentially broad range of experience.
Fundamentally though:
-
More so than more established projects like Wine, we'd like vkd3d (and vkd3d-shader in particular) to be easy to pick up for new/incidental contributors. If that implies spelling out some things that may be obvious to more experienced contributors, that seems fine to me. I.e., this shouldn't turn into a C tutorial, but if it ends up becoming something along the lines of "getting started with vkd3d-shader", that may not be a bad thing.
-
I think vkd3d in general could do with more/better documentation. I'm primarily concerned about user facing documentation like the public API reference, and higher level overviews for how to use vkd3d, vkd3d-shader, and vkd3d-utils in applications or libraries, but I can only encourage anyone willing to work on this.
-
I think it's fine to go through a couple of review rounds to see where improvements can be made before committing these, but I don't think it necessarily needs to be perfect before it can go in. We'd like to avoid comments that are misleading or just wrong, but the potential for regressions is pretty limited, and I'd expect this to evolve a bit after it's committed in any case.
-
I thought I had replied again to this thread, but it seems by reply got lost somehow. Annoying.
Anyway, if @fcasas is willing to keep steering this, I would suggest him to rework his proposal according to the criteria that were discussed. Even before, since also some type definition refactoring was suggested, maybe that could be an even earlier step (like refactoring
struct hlsl_type
, precomputing the name, renamingtype
andbase_type
together with their enums, something else?).
Regarding obvious comments
I will remove the obvious comments in the next version, but I think it is not a waste of time to explain why I wrote them in the first place, maybe there can be some interesting discussion.
I understand that some comments are obvious, but a potential reader may not know if the absence of a comment means that the field should indeed be understood in the most obvious way, or it is not obvious at all and just missing documentation.
Another reason why I added obvious comments is that, sadly, comments are not associated to a portion of the code (like, say, comments in a Google Docs document review), but instead inserted on it. So, sometimes an obvious comment is useful to indicate that a previous comment that explains multiple lines doesn't explain the following one, e.g. :
/* Position of the reader */ unsigned int page; unsigned int character; unsigned int frame_counter;
versus
/* Position of the reader */ unsigned int page; unsigned int character; /* Counter of frames currently displayed */ unsigned int frame_counter;
In some personal projects I used to add comments with merely a
v
to indicate that the following line is self-explanatory, e.g.:/* v */ unsigned int frame_counter;
to address these problems, but I understand that that solution may be too unorthodox. Maybe empty comments are more acceptable, or at least adding empty lines to delimit the scope of the last comment:
/* Position of the reader */ unsigned int page; unsigned int character; unsigned int frame_counter;
Using empty lines to mark blocks of fields that are tied more strongly together is without doubt a good idea. As for obvious comments, I still have the feeling that something like
/* Reader position */ unsigned int reader_position;
it much more likely to distract rather then help a reader, but I agree that's a rather extreme case. I will trust your judgement on there to draw the line.
Regarding the code changes suggested
For convenience I will write a list here of the changes suggested:
- Split
enum hlsl_base_type
into numeric and object types, and use thee
union instruct hlsl_type
to handle both cases separately. Making thedimx
anddimy
fields only for numeric types. - Rename
enum hlsl_type_class
toenum hlsl_type_kind
. - Precompute
hlsl_type_to_string()
always.
I agree with all these changes. However, the first one may require ubiquitous changes and complicate rebases, so I think we better properly coordinate for when we want to implement it. I would prefer to get most of the "broaden resources support" v2 patch series upstreamed first (I am still working on it), because it also modifies core data types, introducing ubiquious changes, and it may get complicated to upstream two of these changes at the same time.
I agree in that these code changes shouldn't freeze documentation patches since, if we comment a piece of code that we will change later, the updates in the documentation will be additional information to make the commits more readable (provided we remember to do update it). This was the reason why I intented to send this patch series before "broaden resources support" v2.
- Split
Regarding general feedback for v2
I think that most of the feedback goes in the direction of shifting from explaining what things are (which can often be extracted from the data type and name of the field) to actually explaining how they are used.
Of course, these type of comments will be more coupled to the rest of the code, but I see now that they are more helpful (as long as we keep them updated).
I can extract the following concerete things:
- Explain the direction of containment relationships and directly refer to the field in the other type that is connected to it.
- Explain where/when the fields/structs are initialized (unless it is obvious).
- Explain where/when the fields are used (unless they are always used).
- Explain how the type is stored (this can be done in the fields in charge of containment relationships).
- Usually the last two are related to a level in the code: source, IR, or bytecode. They may also be related to specific shader models.
- Note when a type is unique, i.e. instanced only once, like hlsl_ctx.
- If a field has a non-trivial data type, assume that the reader can go to its definition and extract more information from it.
I would add: explain what formal rules an object of a type must satisfy. E.g., when this flag is set this field is relevant, otherwise it's not; or when this field is set to this value, then this other field must be non-negative. As usual, taking with a grain of salt if the explanation becomes so convoluted that it doesn't really help.
I think I gave the impression in an earlier comment that "we shouldn't add comments for things that are self-explanatory", which either is not what I meant or is not how I feel now after thinking about it.
I think there shouldn't be an expectation that we need to document every field just for the sake of documenting every field, but if you feel like a field would be improved with documentation, I don't think that there's any reason to bikeshed that. (Not least because, as the most recent person to work on the HLSL compiler, your thoughts should be given the heaviest weight.) Similarly explaining what a thing is doesn't necessarily hurt either. Especially for something like hlsl_deref which is kind of weird and non-obvious. (Similarly I could see the difference between hlsl_ir_function and hlsl_ir_function_decl being confusing.)
mentioned in merge request !50 (merged)
Closing, since it is superseded by !50 (merged).