[PATCH 0/14] MR38: 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:  -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.h | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index f237d6c4..58ad01a5 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -114,33 +114,60 @@ enum hlsl_matrix_majority HLSL_ROW_MAJOR }; +/* An HLSL data type. */ struct hlsl_type { + /* Linked list entry to store this hlsl_type in the context's linked list of hlsl_types. */ struct list entry; + /* Search tree entry to store this hlsl_type in the context's search tree of types. The type's + * name is used as key. */ struct rb_entry scope_entry; + /* These fields indicate to which category of hlsl_types this hlsl_type belongs. + * If type is HLSL_CLASS_OBJECT, base_type is used to indicate a subcategory of the type. + * If type is numeric, base_type is used to indicate the type of its components. + * base_type is not used when type is HLSL_CLASS_STRUCT or HLSL_CLASS_ARRAY. */ enum hlsl_type_class type; enum hlsl_base_type base_type; + /* Sampling dimension of the hlsl_type, in case it is a resource type, e.g. a texture or + * sampler. */ enum hlsl_sampler_dim sampler_dim; + /* String buffer with the name of the hlsl_type. */ const char *name; + /* Bitfield for storing type modifiers, subset of HLSL_TYPE_MODIFIERS_MASK */ unsigned int modifiers; + /* Size of the type values on each dimension; primarily intended for numeric types: scalars, + * vectors, and matrices.Other types may assign values to them for convenience. + * See type initialization functions. */ unsigned int dimx; unsigned int dimy; + union { + /* Additional information if the type is HLSL_CLASS_STRUCT. */ struct { + /* Fields contained within the struct. */ struct hlsl_struct_field *fields; + /* Number of fields contained in the struct. */ size_t field_count; } record; + /* Additional information if the type is HLSL_CLASS_ARRAY. */ struct { + /* Type of the array elements. */ struct hlsl_type *type; + /* Number of elements contained within the array or HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT + * if it is unknown yet. */ unsigned int elements_count; } array; + /* Format of the data contained within the type if it is a resource type, e.g. a texture. */ struct hlsl_type *resource_format; } e; + /* Number of numeric register components used by one value of this type (4 components make 1 + * register). */ unsigned int reg_size; + /* Offset where the type's description starts in the output bytecode. */ size_t bytecode_offset; }; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.h | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 58ad01a5..d17163e0 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -177,15 +177,24 @@ struct hlsl_semantic uint32_t index; }; +/* A field within a struct type declaration. */ struct hlsl_struct_field { + /* Location of the field declaration in the HLSL shader. */ struct vkd3d_shader_location loc; + /* Data type of the field. */ struct hlsl_type *type; + /* Name of the field. */ const char *name; + + /* Semantic linked to this field, if any. */ struct hlsl_semantic semantic; + /* Bitfield for storing variable modifiers that are specific for this field */ unsigned int modifiers; + /* Offset of the field within the type it belongs to, in numeric register components. */ unsigned int reg_offset; + /* Offset where the fields's name starts in the output bytecode. */ size_t name_bytecode_offset; }; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.h | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index d17163e0..54a7c7d5 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -275,19 +275,40 @@ struct hlsl_reg_reservation unsigned int index; }; +/* Variable declared within the HLSL shader. */ struct hlsl_ir_var { + /* Data type of the variable. */ struct hlsl_type *data_type; + /* Location of the variable declaration in the HLSL shader. */ struct vkd3d_shader_location loc; + /* Name of the variable. */ const char *name; + /* Semantic linked to the variable, if any. */ struct hlsl_semantic semantic; + /* Reference to the buffer where the variable's value is stored, in case it is uniform. */ struct hlsl_buffer *buffer; + /* Bitfield for storing variable modifiers */ unsigned int modifiers; + /* Optional register assignment where the variable has to be allocated. */ struct hlsl_reg_reservation reg_reservation; + /* Nodes for storing this variable in: + * - The scope's variable list (may be hlsl_ctx's global scope). + * - An hlsl_ir_function_decl's parameters list. + * - The hlsl_ctx's externs list. + * respectively, when it applies. */ struct list scope_entry, param_entry, extern_entry; + /* Indexes of the IR instructions where the variable is first written and last read (liveness + * range). The IR instructions are numerated starting from 2, because 0 means unused, and 1 + * means function entry. */ unsigned int first_write, last_read; + /* Offset where the variable's value is stored within its buffer, in numeric register + * components. */ unsigned int buffer_offset; + /* Register to which variable is allocated during its lifetime. + * In case that the variable uses more than one (continuous) register, this refers to the first + * one. */ struct hlsl_reg reg; uint32_t is_input_semantic : 1; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.h | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 54a7c7d5..95ed5a79 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -532,49 +532,74 @@ struct hlsl_buffer struct hlsl_ctx { + /* Information about the compilation's target profile. */ const struct hlsl_profile_info *profile; + /* Array of pointers to the names of the source files. */ const char **source_files; unsigned int source_files_count; + /* Current location being read, updated while parsing. */ struct vkd3d_shader_location location; + /* Stores the logging messages and logging configuration. */ struct vkd3d_shader_message_context *message_context; + /* Stores string buffers currently in use. */ struct vkd3d_string_buffer_cache string_buffers; + /* The result of the compilation (a value from vkd3d_result). 0 means success. */ int result; + /* Pointer to an opaque data structure managed by FLEX (during lexing), that encapsulates the + * current state of the scanner. This pointer is required by all FLEX API functions when the + * scanner is declared as reentrant, which is the case. */ void *scanner; + /* Pointer to the current scope; changes as the parser reads the code. */ struct hlsl_scope *cur_scope; + /* Scope of global variables. */ struct hlsl_scope *globals; + /* List that contains all the scopes in the program. */ struct list scopes; + /* List that contains the extern variables of the program. */ struct list extern_vars; + /* List that includes the built-in HLSL buffers (globals and parameters), as well as the ones + * declared in the shader. */ struct list buffers; + /* Current buffer (changes as the parser reads the code), $Globals buffer and $Params buffer. */ struct hlsl_buffer *cur_buffer, *globals_buffer, *params_buffer; + /* List containing all created hlsl_types, excluding builtin_types. */ struct list types; + /* Search tree of declared functions. The function's name is used as key. */ struct rb_tree functions; + /* Pointer to the current function; changes as the parser reads the code. */ const struct hlsl_ir_function_decl *cur_function; + /* Default matrix majority for matrix types. Can be set by a pragma within the HLSL code. */ enum hlsl_matrix_majority matrix_majority; + /* Basic data types stored for convenience. */ struct { struct hlsl_type *scalar[HLSL_TYPE_LAST_SCALAR + 1]; struct hlsl_type *vector[HLSL_TYPE_LAST_SCALAR + 1][4]; - /* matrix[float][2][4] is a float4x2, i.e. dimx = 2, dimy = 4 */ + /* matrix[float][1][3] is a float4x2, i.e. dimx = 2, dimy = 4 */ struct hlsl_type *matrix[HLSL_TYPE_LAST_SCALAR + 1][4][4]; struct hlsl_type *sampler[HLSL_SAMPLER_DIM_LAST_SAMPLER + 1]; struct hlsl_type *Void; } builtin_types; + /* List of instructions that initialize static variables. */ struct list static_initializers; + /* Dynamic array of constant values that appear in the shader. Only used for SM1 profiles. */ struct hlsl_constant_defs { struct hlsl_vec4 *values; size_t count, size; } constant_defs; + /* Number of temporary registers required by the shader. */ uint32_t temp_count; + /* Whether the parser is inside an state block (effects' metadata) inside a variable declarition. */ uint32_t in_state_block : 1; }; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.h | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 95ed5a79..a0d5ebe1 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -205,6 +205,8 @@ struct hlsl_reg bool allocated; }; +/* Type of IR instruction node. All instruction node types are associated to a struct with the same + * name (in lower case). */ enum hlsl_ir_node_type { HLSL_IR_CONSTANT, @@ -218,14 +220,24 @@ enum hlsl_ir_node_type HLSL_IR_SWIZZLE, }; +/* Contains the data common to all instruction node types. All of the structs associated to each + * instruction node type contain a struct hlsl_ir_node as its first member, so pointers to these + * structs can be casted seamlessly to (struct hlsl_ir_node *) and vice-versa. */ struct hlsl_ir_node { + /* Linked list entry, used for inserting this instruction node on any linked list of instruction + * nodes as required. */ struct list entry; + /* Type of node, which means that a pointer to this struct hlsl_ir_node can be casted to a + * pointer to the struct with the same name. */ enum hlsl_ir_node_type type; + /* HLSL data type of the node, when used as an expression. */ struct hlsl_type *data_type; + /* Linked list of all the struct hlsl_src-s that point to this node. */ struct list uses; + /* Location associated to this instruction within the HLSL shader. */ struct vkd3d_shader_location loc; /* Liveness ranges. "index" is the index of this instruction. Since this is @@ -233,6 +245,7 @@ struct hlsl_ir_node * true even for loops, since currently we can't have a reference to a * value generated in an earlier iteration of the loop. */ unsigned int index, last_read; + /* Temp. register allocated to store the result of this instruction (if any). */ struct hlsl_reg reg; }; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index a0d5ebe1..e8d804a2 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -254,6 +254,9 @@ struct hlsl_block struct list instrs; }; +/* A reference to an instruction node (hlsl_ir_node), used by other instructions. Besides a pointer + * to this instruction node, this struct also contains a linked list entry, which allows the instruction + * node to keep track of all the hlsl_srcs that reference it. */ struct hlsl_src { struct hlsl_ir_node *node; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index e8d804a2..36125cdf 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -198,6 +198,9 @@ struct hlsl_struct_field size_t name_bytecode_offset; }; +/* Information of the register allocated for an instruction node or variable. + * The type of register is implied from the HLSL data type of the instruction/variable, so it is + * not present in this struct. */ struct hlsl_reg { uint32_t id; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.h | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 36125cdf..bf65dbcc 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -451,13 +451,26 @@ struct hlsl_ir_swizzle DWORD swizzle; }; +/* Reference to a variable, or a part of it (e.g. a vector within a matrix within a struct). */ struct hlsl_deref { + /* Pointer to the referenced variable */ struct hlsl_ir_var *var; + /* An array of references to instruction nodes, of HLSL type uint, that are used to reach + * the desired part of the variable. + * If path_len is 0, then this is a reference to the whole variable. + * The value of each instruction node in the path corresponds to the index of the element/field + * that has to be selected on each nesting level to reach this part. + * The path shall not contain additional values once a type that cannot be subdivided + * (a.k.a. "component") is reached. */ unsigned int path_len; struct hlsl_src *path; + /* Single instruction node of HLSL type uint used to represent the register offset (in register + * components), from the start of the variable, of the part referenced. + * Currently, the path is lowered to this single offset before translating the dereferences to + * the target binary format. */ struct hlsl_src offset; }; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index bf65dbcc..e22d0090 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -171,6 +171,8 @@ struct hlsl_type size_t bytecode_offset; }; +/* In HLSL, a semantic is a string linked to a variable (or a field) to be recognized across + * different shader stages in the graphics pipeline. */ struct hlsl_semantic { const char *name; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.h | 1 + 1 file changed, 1 insertion(+) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index e22d0090..1b5c44a4 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -254,6 +254,7 @@ struct hlsl_ir_node struct hlsl_reg reg; }; +/* Container of instruction nodes (hlsl_ir_node). */ struct hlsl_block { struct list instrs; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.h | 1 + 1 file changed, 1 insertion(+) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 1b5c44a4..06c9aefc 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -291,6 +291,7 @@ struct hlsl_src #define HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT 0 +/* The reservation, written in the HLSL shader, of a specific register to a variable (or a field). */ struct hlsl_reg_reservation { char type; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 06c9aefc..1bf0a964 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -554,15 +554,25 @@ enum hlsl_buffer_type HLSL_BUFFER_TEXTURE, }; +/* HLSL buffer to group a set of uniform variables. */ struct hlsl_buffer { + /* Location of the buffer declaration in the HLSL shader. */ struct vkd3d_shader_location loc; + /* Type of buffer. */ enum hlsl_buffer_type type; + /* Name of the buffer */ const char *name; + /* Used to reserve a particular register as the starting point for the variables' values. */ struct hlsl_reg_reservation reservation; + /* Linked list entry for storing the buffer in the context's list of buffers. */ struct list entry; + /* The size of the buffer (in register components), and the size of the buffer as determined + * by its last variable that's actually used. */ unsigned size, used_size; + /* Register on which the buffer is allocated. + * In case that the buffer uses more than one register, this refers to the first one. */ struct hlsl_reg reg; }; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 1bf0a964..0dc5254d 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -340,23 +340,39 @@ struct hlsl_ir_var uint32_t is_param : 1; }; +/* A HLSL function. */ struct hlsl_ir_function { + /* Entry for the function in the context's search tree. */ struct rb_entry entry; + /* Name of the function. */ const char *name; + /* Search tree for the overloads of this function, as hlsl_ir_function_decls. + * The parameters' types are used as key. */ struct rb_tree overloads; + /* Whether the function is intrinsic or not, if we ever want to store an intrinsic function + * within the context's search tree of functions. */ bool intrinsic; }; +/* A specific HLSL function definition, with specific parameter types. */ struct hlsl_ir_function_decl { + /* Type of the returnd value. */ struct hlsl_type *return_type; + /* Pointer to a synthetic variable used to store the return value of the function. */ struct hlsl_ir_var *return_var; + /* Location of the declaration in the HLSL shader. */ struct vkd3d_shader_location loc; + /* Entry for the declaration within the hlsl_ir_function's search tree of overloads. */ struct rb_entry entry; + /* Pointer to the hlsl_ir_function to which this declaration corresponds. */ struct hlsl_ir_function *func; + /* List of variables. One variable is created for each parameter of the function. */ struct list *parameters; + /* Instructions within the body of the function. */ struct hlsl_block body; + /* Whether the function declaration contains a definition with instructions. */ bool has_body; }; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 0dc5254d..aa682c1e 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -540,11 +540,17 @@ struct hlsl_ir_constant struct hlsl_reg reg; }; +/* A delimited section of code, that contains variable and type defintions. */ struct hlsl_scope { + /* Entry for the context's list of scopes. */ struct list entry; + /* List of variables declared inside this scope. */ struct list vars; + /* List of HLSL types declared inside this scope. */ struct rb_tree types; + /* Scope that contains this scope. If definitions aren't found within this scope, the upper + * scope is checked, and so on until the global scope is reached. */ struct hlsl_scope *upper; }; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.h:
HLSL_ROW_MAJOR };
+/* An HLSL data type. */ struct hlsl_type
Here and in other places the comment is quite obvious and doesn't help a lot. I would just add a comment when it adds something that was not evident before. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_12041
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.h:
HLSL_ROW_MAJOR };
+/* An HLSL data type. */ struct hlsl_type { + /* Linked list entry to store this hlsl_type in the context's linked list of hlsl_types. */ struct list entry; 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 which `struct 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` and `struct rb_tree`. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_12042
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.h:
};
+/* An HLSL data type. */ struct hlsl_type { + /* Linked list entry to store this hlsl_type in the context's linked list of hlsl_types. */ struct list entry; + /* Search tree entry to store this hlsl_type in the context's search tree of types. The type's + * name is used as key. */ struct rb_entry scope_entry; + /* These fields indicate to which category of hlsl_types this hlsl_type belongs. + * If type is HLSL_CLASS_OBJECT, base_type is used to indicate a subcategory of the type. + * If type is numeric, base_type is used to indicate the type of its components. + * base_type is not used when type is HLSL_CLASS_STRUCT or HLSL_CLASS_ARRAY. */ enum hlsl_type_class type; enum hlsl_base_type base_type; 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.
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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_12043
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.h:
+ /* Linked list entry to store this hlsl_type in the context's linked list of hlsl_types. */ struct list entry; + /* Search tree entry to store this hlsl_type in the context's search tree of types. The type's + * name is used as key. */ struct rb_entry scope_entry; + /* These fields indicate to which category of hlsl_types this hlsl_type belongs. + * If type is HLSL_CLASS_OBJECT, base_type is used to indicate a subcategory of the type. + * If type is numeric, base_type is used to indicate the type of its components. + * base_type is not used when type is HLSL_CLASS_STRUCT or HLSL_CLASS_ARRAY. */ enum hlsl_type_class type; enum hlsl_base_type base_type; + /* Sampling dimension of the hlsl_type, in case it is a resource type, e.g. a texture or + * sampler. */ enum hlsl_sampler_dim sampler_dim; + /* String buffer with the name of the hlsl_type. */ const char *name; That seems to be quite obvious.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_12044
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.h:
unsigned int modifiers; + /* Size of the type values on each dimension; primarily intended for numeric types: scalars, + * vectors, and matrices.Other types may assign values to them for convenience. + * See type initialization functions. */ unsigned int dimx; unsigned int dimy; + union { + /* Additional information if the type is HLSL_CLASS_STRUCT. */ struct { + /* Fields contained within the struct. */ struct hlsl_struct_field *fields; + /* Number of fields contained in the struct. */ size_t field_count;
These two seem pretty obvious. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_12045
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.h:
+ /* Additional information if the type is HLSL_CLASS_STRUCT. */ struct { + /* Fields contained within the struct. */ struct hlsl_struct_field *fields; + /* Number of fields contained in the struct. */ size_t field_count; } record; + /* Additional information if the type is HLSL_CLASS_ARRAY. */ struct { + /* Type of the array elements. */ struct hlsl_type *type; + /* Number of elements contained within the array or HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT + * if it is unknown yet. */ unsigned int elements_count; These seem obvious too, except for an indication that the indication that `HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT` is used when the element count is unknown.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_12046
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.h:
struct hlsl_type { + /* Linked list entry to store this hlsl_type in the context's linked list of hlsl_types. */ struct list entry; + /* Search tree entry to store this hlsl_type in the context's search tree of types. The type's + * name is used as key. */ struct rb_entry scope_entry; + /* These fields indicate to which category of hlsl_types this hlsl_type belongs. + * If type is HLSL_CLASS_OBJECT, base_type is used to indicate a subcategory of the type. + * If type is numeric, base_type is used to indicate the type of its components. + * base_type is not used when type is HLSL_CLASS_STRUCT or HLSL_CLASS_ARRAY. */ enum hlsl_type_class type; enum hlsl_base_type base_type; + /* Sampling dimension of the hlsl_type, in case it is a resource type, e.g. a texture or + * sampler. */ enum hlsl_sampler_dim sampler_dim; 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` is `HLSL_TYPE_SAMPLER` then these are the allowed values: ...; if `base_type` is `HLSL_TYPE_TEXTURE` then these are the allowed values: ...; etc.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_12047
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.h:
+ * If type is numeric, base_type is used to indicate the type of its components. + * base_type is not used when type is HLSL_CLASS_STRUCT or HLSL_CLASS_ARRAY. */ enum hlsl_type_class type; enum hlsl_base_type base_type; + /* Sampling dimension of the hlsl_type, in case it is a resource type, e.g. a texture or + * sampler. */ enum hlsl_sampler_dim sampler_dim; + /* String buffer with the name of the hlsl_type. */ const char *name; + /* Bitfield for storing type modifiers, subset of HLSL_TYPE_MODIFIERS_MASK */ unsigned int modifiers; + /* Size of the type values on each dimension; primarily intended for numeric types: scalars, + * vectors, and matrices.Other types may assign values to them for convenience. + * See type initialization functions. */ unsigned int dimx; unsigned int dimy; Here too it is useful to know the precise rules for what `dimx` and `dimy` are set to depending on the `base_type`. Saying "See type initialization functions" kind of contradicts your intention of documenting data structures rather than code.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_12048
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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_12049
On Tue Oct 25 15:14:19 2022 +0000, Giovanni Mascellani wrote:
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 which `struct 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` and `struct rb_tree`. Agreed in general. I think the phrasing I'd use is "entry in hlsl_ctx->types", but I'm not picky.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_12170
On Tue Oct 25 15:14:20 2022 +0000, Giovanni Mascellani wrote:
That seems to be quite obvious. 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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_12171
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 ```c 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 ;-) -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_12172
On Tue Oct 25 15:14:21 2022 +0000, Giovanni Mascellani wrote:
Here too it is useful to know the precise rules for what `dimx` and `dimy` are set to depending on the `base_type`. Saying "See type initialization functions" kind of contradicts your intention of documenting data structures rather than code. 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).
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_12173
On Wed Oct 26 05:39:19 2022 +0000, Zebediah Figura wrote:
Agreed in general. I think the phrasing I'd use is "entry in hlsl_ctx->types", but I'm not picky. 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.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_12183
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". -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_12197
On Wed Oct 26 05:39:19 2022 +0000, Zebediah Figura wrote:
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. Agree to precompute.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_12198
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 ;-) -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_12309
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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_12331
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, renaming `type` and `base_type` together with their enums, something else?). -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_12366
On Fri Oct 28 19:24:08 2022 +0000, Giovanni Mascellani wrote:
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, renaming `type` and `base_type` together with their enums, something else?). Maybe, but I'm also not sure it needs to block this patch. In line with what Henri said, there's no harm in having some of this documentation be partial or temporary.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_12397
On Fri Oct 28 19:33:16 2022 +0000, Henri Verbeet wrote:
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 agree with these points.
I would only suggest that this series be limited a bit from 14 patches, since otherwise reviewing all of them will take a lot longer. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_12403
Okay, I am writing v2 trying to follow the guidelines presented in your nice feedback. I will post my replies to some of your suggestions in separate comments so that we don't discuss everything in the same thread. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_13950
## 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. : ```c /* Position of the reader */ unsigned int page; unsigned int character; unsigned int frame_counter; ``` versus ```c /* 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.: ```c /* 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: ```c /* Position of the reader */ unsigned int page; unsigned int character; unsigned int frame_counter; ``` -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_13951
## 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 the `e` union in `struct hlsl_type` to handle both cases separately. Making the `dimx` and `dimy` fields only for numeric types. * Rename `enum hlsl_type_class` to `enum 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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_13952
## 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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_13953
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.) -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_14028
On Fri Nov 11 09:45:16 2022 +0000, Francisco Casas wrote:
## 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. : ```c /* Position of the reader */ unsigned int page; unsigned int character; unsigned int frame_counter; ``` versus ```c /* 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.: ```c /* 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: ```c /* 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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_14037
On Fri Nov 11 09:46:57 2022 +0000, Francisco Casas wrote:
## 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 the `e` union in `struct hlsl_type` to handle both cases separately. Making the `dimx` and `dimy` fields only for numeric types. * Rename `enum hlsl_type_class` to `enum 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. Ok, it makes sense to concentrate on documentation now and leave code changes to later. Also because I am not sure they would be accepted, given that we are in code freeze.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_14038
On Fri Nov 11 09:50:15 2022 +0000, Francisco Casas wrote:
## 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.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_14039
This merge request was closed by Francisco Casas. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38
Closing, since it is superseded by !50. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/38#note_16106
participants (5)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)
-
Zebediah Figura (＠zfigura)