xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Jan Beulich <jbeulich@suse.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: Luca Fancellu <luca.fancellu@arm.com>,
	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 10:10:56 +0200	[thread overview]
Message-ID: <e15ed7b9-b153-b767-e625-e7a9d2aa76bf@suse.com> (raw)
In-Reply-To: <alpine.DEB.2.21.2104061425040.31460@sstabellini-ThinkPad-T480s>

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.

Jan


  reply	other threads:[~2021-04-07  8:11 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 [this message]
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
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=e15ed7b9-b153-b767-e625-e7a9d2aa76bf@suse.com \
    --to=jbeulich@suse.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=bertrand.marquis@arm.com \
    --cc=george.dunlap@citrix.com \
    --cc=iwj@xenproject.org \
    --cc=julien@xen.org \
    --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).