Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> On 8 Apr 2021, at 12:50, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> On 08/04/2021 12:40, Julien Grall wrote:
>> Hi Luca,
>> On 08/04/2021 12:02, Luca Fancellu wrote:
>>> 
>>> 
>>>> On 7 Apr 2021, at 22:26, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
>>>> 
>>>> On Wed, 7 Apr 2021, Jan Beulich wrote:
>>>>> On 07.04.2021 10:42, Luca Fancellu wrote:
>>>>>> Just to be sure that we are in the same page, are you suggesting to 
>>>>>> modify the name
>>>>>> In this way?
>>>>>> 
>>>>>> struct gnttab_cache_flush {
>>>>>> -    union {
>>>>>> +    union xen_gnttab_cache_flush_a {
>>>>>>         uint64_t dev_bus_addr;
>>>>>>         grant_ref_t ref;
>>>>>>     } a;
>>>>>> 
>>>>>> Following this kind of pattern: xen_<upper struct name>_<member name> ?
>>>>> 
>>>>> While in general I would be fine with this scheme, for field names like
>>>>> "a" or "u" it doesn't fit well imo.
>>>> 
>>>> "a" is a bad name anyway, even for the member. We can take the
>>>> opportunity to find a better name. Almost anything would be better than
>>>> "a". Maybe "refaddr"?
>>>> 
>>>> 
>>>>> I'm also unconvinced this would be
>>>>> scalable to the case where there's further struct/union nesting.
>>>> 
>>>> How many of these instances of multilevel nesting do we have? Luca might
>>>> know. Probably not many? They could be special-cased.
>>> 
>>> There are not many multilevel nesting instances of anonymous struct/union 
>>> and the maximum level of nesting I found in the public headers is 2:
>>> 
>>> union {
>>>     union/struct {
>>>         …
>>>     } <name>
>>> } <name>
>>> 
>>> I also see that in the majority of cases the unions have not meaningful 
>>> names like “a” or “u” as member name, instead struct names are fine,
>>> It could be fine to keep the meaningful name the same for the struct type 
>>> name and use the pattern for the non-meaningful ones as long
>>> as the names doesn’t create compilation errors?
>>> 
>>> Example:
>>> 
>>> struct upper_level {
>>>     union {
>>>         struct {
>>> 
>>>         } meaningful_name1;
>>>         struct {
>>> 
>>>         } meaningful_name2;
>>>     } u;
>>> };
>>> 
>>> becomes:
>>> 
>>> struct upper_level {
>>>     union upper_level_u {
>>>         struct meaningful_name1 {
>>> 
>>>         } meaningful_name1;
>>>         struct meaningful_name2 {
>>> 
>>>         } meaningful_name2;
>>>     } u;
>>> };
>> If I understand correctly your proposal, the name of the structure would be 
>> the name of the field. The name of the fields are usually pretty generic so 
>> you will likely end up to redefine the structure name.
>> Unless we want to provide random name, the only safe naming would be to 
>> define the structure as upper_level_u_meaningful_name{1, 2}. But, this is 
>> going to be pretty awful to read.
>> But I am still a bit puzzled by the fact doxygen is not capable to deal with 
>> anynomous/unamed union. How do other projects deal with them?
> 
> While going through the list of anynomous union in Xen, I noticed we also 
> have something like:
> 
> struct test {
>    union {
>        int a;
>        int b;
>    };
> };
> 
> We can't name them because of syntactic reasons. What's your plan for them?

I would say that if the fields a and b are not meant to be described in the 
documentation, they can be hidden, so there is no need to change the structure 
itself but we might just add some comment containing
Doxygen tags for skipping them.
In Zephyr they have some kind of structures like that.

> 
> Cheers,
> 
> -- 
> Julien Grall