xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Julien Grall <julien@xen.org>
To: Luca Fancellu <luca.fancellu@arm.com>,
	Stefano Stabellini <sstabellini@kernel.org>
Cc: Jan Beulich <jbeulich@suse.com>,
	Bertrand Marquis <bertrand.marquis@arm.com>,
	wei.chen@arm.com, Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Ian Jackson <iwj@xenproject.org>, Wei Liu <wl@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h
Date: Thu, 8 Apr 2021 12:40:13 +0100	[thread overview]
Message-ID: <e3edf002-9d60-7836-f7f0-cd9877653c2c@xen.org> (raw)
In-Reply-To: <D0931FDE-29DA-41B0-87AF-EF18ABDDA37E@arm.com>

Hi Luca,

On 08/04/2021 12:02, Luca Fancellu wrote:
> 
> 
>> On 7 Apr 2021, at 22:26, Stefano Stabellini <sstabellini@kernel.org> 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?

> 
> Doing this will help a lot the documentation side because the html page will show the "struct upper_level" with inside the “union upper_level_u" and inside again
> the two struct “meaningful_name1" and “meaningful_name2", and from the point of view of the developer it can tell her/him exactly the name of the member to
> access when writing code (apart from the upper_level_u that can be accessed by u, but we can’t clearly change it).

I don't quite understand the last point. Wouldn't the developper see the 
field name? So how is it going to be different from seeing the structure 
name?

> If this sounds difficult to understand when reading, please generate the documentation and have a look on the page in one side and the source code in another.

Just to be clear, do you mean understanding what you wrote or a 
developper trying to understand the code?

Cheers,

-- 
Julien Grall


  parent reply	other threads:[~2021-04-08 11:40 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-06 10:36 [PATCH 0/3] Use Doxygen and sphinx for html documentation Luca Fancellu
2021-04-06 10:36 ` [PATCH 1/3] docs: add doxygen support " Luca Fancellu
2021-04-06 10:36 ` [PATCH 2/3] docs: hypercalls sphinx skeleton for generated html Luca Fancellu
2021-04-06 10:36 ` [PATCH 3/3] docs/doxygen: doxygen documentation for grant_table.h Luca Fancellu
2021-04-06 11:19   ` Jan Beulich
2021-04-06 21:46     ` Stefano Stabellini
2021-04-07  8:10       ` Jan Beulich
2021-04-07  8:42         ` Luca Fancellu
2021-04-07  8:58           ` Jan Beulich
2021-04-07 21:26             ` Stefano Stabellini
2021-04-08  5:59               ` Jan Beulich
2021-04-08 11:02               ` Luca Fancellu
2021-04-08 11:28                 ` Jan Beulich
2021-04-08 11:40                 ` Julien Grall [this message]
2021-04-08 11:50                   ` Julien Grall
2021-04-08 13:13                     ` Luca Fancellu
2021-04-08 11:58                   ` Luca Fancellu
2021-04-07 13:13   ` Julien Grall
2021-04-07 13:19     ` Luca Fancellu
2021-04-07 13:56       ` Julien Grall
2021-04-07 14:51         ` Luca Fancellu
2021-04-07 15:19       ` Ian Jackson
2021-04-07 15:29         ` Bertrand Marquis
2021-04-07 15:54           ` Jan Beulich
2021-04-07 16:07             ` Bertrand Marquis
2021-04-07 15:55           ` Julien Grall
2021-04-07 16:06             ` Bertrand Marquis
2021-04-07 16:12               ` Ian Jackson
2021-04-06 11:53 ` [PATCH 0/3] Use Doxygen and sphinx for html documentation Andrew Cooper
2021-04-06 21:24   ` Stefano Stabellini
2021-04-07 11:15     ` Andrew Cooper
2021-04-07 13:05       ` Bertrand Marquis
2021-04-07 13:07 ` Julien Grall
2021-04-07 13:16   ` Luca Fancellu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e3edf002-9d60-7836-f7f0-cd9877653c2c@xen.org \
    --to=julien@xen.org \
    --cc=andrew.cooper3@citrix.com \
    --cc=bertrand.marquis@arm.com \
    --cc=george.dunlap@citrix.com \
    --cc=iwj@xenproject.org \
    --cc=jbeulich@suse.com \
    --cc=luca.fancellu@arm.com \
    --cc=sstabellini@kernel.org \
    --cc=wei.chen@arm.com \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).