xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Luca Fancellu <luca.fancellu@arm.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	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>, Julien Grall <julien@xen.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: Wed, 7 Apr 2021 09:42:23 +0100	[thread overview]
Message-ID: <DF5BE432-F79E-4874-92DF-60A4DCD9EDBD@arm.com> (raw)
In-Reply-To: <e15ed7b9-b153-b767-e625-e7a9d2aa76bf@suse.com>



> On 7 Apr 2021, at 09:10, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 06.04.2021 23:46, Stefano Stabellini wrote:
>> On Tue, 6 Apr 2021, Jan Beulich wrote:
>>> On 06.04.2021 12:36, Luca Fancellu wrote:
>>>> Modification to include/public/grant_table.h:
>>>> 
>>>> 1) Change anonymous structure to be named structure,
>>>>   because doxygen can't deal with them.
>>> 
>>> Especially in the form presented (adding further name space clutter
>>> for consumers to fall over) I object to this, most notably ...
>>> 
>>>> @@ -584,7 +599,7 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t);
>>>>  * page granted to the calling domain by a foreign domain.
>>>>  */
>>>> struct gnttab_cache_flush {
>>>> -    union {
>>>> +    union a {
>>> 
>>> ... this one.
>> 
>> It is unfortunate that none of these tools support anonymous structs or
>> unions well. (You might recall we also had issues with the older
>> kernel-doc series too, although a bit different.)
> 
> While I wouldn't veto such changes, I think it is a very bad approach
> to make adjustments like this to cover for a docs tool shortcoming.
> Is it entirely unreasonable to have the tool fixed? In fact, if the
> issue was run into before, isn't it a bad sign that the tool hasn't
> been fixed already, and we merely need to require a certain minimum
> version?
> 
>> It is difficult to provide a good name here, a suggestion would be more
>> than welcome. I agree with you that calling it "a" is a bad idea: "a"
>> becomes a globally-visible union name.
>> 
>> Maybe we could call it: StructName_MemberName, so in this case:
>> 
>>  union gnttab_cache_flush_a
>> 
>> It makes sure it is unique and doesn't risk clashing with anything else.
>> We can follow this pattern elsewhere as well.
>> 
>> Any better suggestions?
> 
> First and foremost any new additions ought to use a xen_, Xen_, or
> XEN_ prefix. For the specific case here, since "a" is already a
> rather bad choice for a member name (What does it stand for? In lieu
> of any better name we typically use "u" in such cases.), the badness
> shouldn't be extended. Sadly the ref as being one way of expressing
> the target MFN is also not accompanied by a GFN (as it ought to be),
> but an address. Otherwise I would have suggested to abstract the
> similar union also used by struct gnttab_copy. In the end I can't
> see many alternatives to something like xen_gnttab_cache_flush_target
> or xen_gnttab_cache_flush_ref_or_addr.

Hi Jan,

Thank you for your feedback, I agree with you all that “a” is not really a good name,
I will be happy to change it if we define a pattern.

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> ?

Cheers,
Luca

> 
> Jan



  reply	other threads:[~2021-04-07  8:43 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 [this message]
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
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=DF5BE432-F79E-4874-92DF-60A4DCD9EDBD@arm.com \
    --to=luca.fancellu@arm.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=bertrand.marquis@arm.com \
    --cc=george.dunlap@citrix.com \
    --cc=iwj@xenproject.org \
    --cc=jbeulich@suse.com \
    --cc=julien@xen.org \
    --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).