[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] docs: fix documentation of enum constants



On Fri, 21 Jul 2017 12:58:55 +0200
Michal Privoznik <mprivozn redhat com> wrote:

> On 07/21/2017 12:25 PM, Tomáš Golembiovský wrote:
> > On Fri, 21 Jul 2017 10:12:38 +0200
> > Michal Privoznik <mprivozn redhat com> wrote:
> >   
> >> On 07/20/2017 08:21 PM, Tomáš Golembiovský wrote:  
> >>> The documentation string has to follow the definition of a constant in
> >>> the enum. Otherwise, the HTML documentation will be generated
> >>> incorrectly.
> >>>
> >>> Signed-off-by: Tomáš Golembiovský <tgolembi redhat com>
> >>> ---
> >>>  include/libvirt/libvirt-domain.h | 62 ++++++++++++++++++++--------------------
> >>>  1 file changed, 31 insertions(+), 31 deletions(-)
> >>>
> >>> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> >>> index 45f939a8c..2f3162d0f 100644
> >>> --- a/include/libvirt/libvirt-domain.h
> >>> +++ b/include/libvirt/libvirt-domain.h
> >>> @@ -583,56 +583,56 @@ typedef virDomainInterfaceStatsStruct *virDomainInterfaceStatsPtr;
> >>>   * Memory Statistics Tags:
> >>>   */
> >>>  typedef enum {
> >>> -    /* The total amount of data read from swap space (in kB). */
> >>>      VIR_DOMAIN_MEMORY_STAT_SWAP_IN         = 0,
> >>> -    /* The total amount of memory written out to swap space (in kB). */
> >>> +    /* The total amount of data read from swap space (in kB). */
> >>>      VIR_DOMAIN_MEMORY_STAT_SWAP_OUT        = 1,
> >>> +    /* The total amount of memory written out to swap space (in kB). */    
> >>
> >> While this fixes generated HTML, it messes up the header file. So if
> >> somebody is looking directly into header file they might get confused.
> >> The problem is in our doc generator.  
> > 
> > I agree that it's not ideal solution. But since it's already used in the
> > header, e.g. in virDomainBlockJobType and
> > virDomainEventShutdownDetailType,   
> 
> This one actually looks okay. Did you perhaps mean
> virConnectDomainEventDiskChangeReason?

No, I really meant virDomainEventShutdownDetailType. But you're right
about virConnectDomainEventDiskChangeReason too.

I didn't look at other headers, but as far as libvirt-domain.h is
concerned those three seem to be all. There are also several (5?)
misplaced comments for the sentinels if you care about those too.


> > I assumed it's acceptable. And the
> > overall readability is not that bad because the constant+doc pairs are
> > separated with newline from one another.  
> 
> Nope. It's not. I've sent a patch that fixes those two places (I've went
> through all of our public headers and found just those two though):
> 
> https://www.redhat.com/archives/libvir-list/2017-July/msg00871.html
> 
> > 
> > 
> >   
> >> I recall this being discussed
> >> somewhere recently (probably on the list?). The proper fix IMO is to fix
> >> the generator so that it accepts both:  
> > 
> > That would be the best solution, but I'm too scared to look at the
> > generator. That would be a job for somebody else.  
> 
> Yeah. Our generator is not that great. I wish that we'd switch to
> something already out there so that we don't have to maintain our own
> generator.
> 
> Michal


-- 
Tomáš Golembiovský <tgolembi redhat com>


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]