[PATCH vkd3d 09/17] vkd3d-shader/hlsl: Replace register offsets with index paths in load initializations.

[Zebediah Figura]

On 7/19/22 15:21, Francisco Casas wrote:
> Hello,
> 
> On 19-07-22 05:11, Giovanni Mascellani wrote:
>> Hi,
>>
>> Il 15/07/22 03:23, Francisco Casas ha scritto:
>>> +/* Given a type and a component index, retrieves next path index
>>> required to reach the component.
>>> + * *typep will be set to the subtype within the original type that
>>> contains the component.
>>> + * *indexp will be set to the index of the component within *typep.
>>> + */
>>> +static unsigned int subtype_index_from_component_index(struct
>>> hlsl_ctx *ctx,
>>> +        struct hlsl_type **typep, unsigned int *indexp)
>>
>> I guess that the "p"'s in "typep" and "indexp" are a sort of reverse
>> Hungarian notation. It's a nitpick, but I am not really a fan of that,
>> and I don't think we're using that anywhere in the HLSL compiler.
>>
> 
> I didn't think on Hungarian notation, but I indeed added the 'p' to
> indicate that this is a "pointer to" the actual value.
> 
> Because the referenced values (*typep and *indexp) have to be used
> several times, and typep is a double pointer, I assigned these values to
> local variables:
> 
> struct hlsl_type *type = *typep;
> unsigned int index = *indexp;
> 
> Which is my opinion makes it far more readable that constantly using the
> dereference operator.
> 
> The problem is that I had to pick a different name for the local
> variables an the function arguments.
> 
> But, unless there is another suggestion, in v3 I am renaming the
> pointers "typep" and "indexp" to "type" and "index" respectively, and
> the values from "type" and "index" to "type_val" and "index_val"
> respectively.

Personally I prefer the former, and don't particularly mind the -p 
suffix. Perhaps "type_ptr" would be more palatable.

>>> +/* Returns the path of a given component within a type, given its index.
>>> + * *path_len will be set to the lenght of the path.
>>> + * Memory should be free afterwards.
>>> + */
>>
>> I find the verbal tenses rather confusing: here we have a present tense,
>> a future tense with "will" and a "should". Combined with the typo in
>> "free" (I guess that was "freed"?), it makes the whole lot rather hard
>> to understand if you don't already know what it means.
>>
>> I would suggest to describe what this function does using consistently
>> either an imperative form or a present tense, and to describe
>> expectations from the caller by clearly marking them as such.
>>
>> For example: "Return the path of a given component within a type, given
>> its index, and set *path_len to the length of the path. The caller is
>> responsible for freeing the returned pointer with hlsl_free()".
>>
>> (Many technical writing indications suggest to avoid passive voices to
>> make phrasing clearer; I am not sure that should be a general rule, but
>> it can be a point to keep in mind)
>>
>> BTW, the same suggestion applies to the comment before
>> subtype_index_from_component_index(), though in that case the potential
>> for confusion is smaller.
>>
> 
> Understood, I will prefer present sense and active voice from now on.
> 
> The function compute_component_path() will be removed in v3, but I am
> writing the comment in subtype_index_from_component_index() as:
> 
> /* Given a type and a component index, this function returns the next
>    * path index required to reach the component within the type.
>    * It sets *type to the subtype within the original type that contains
>    * the component.
>    * It sets *index to the index of the component within *type. */

FWIW, I don't see any problem with using future tense and passive voice 
in comments. For external facing documentation it's probably better to 
be consistent, but I don't think we should worry excessively about tense 
and grammar internally.