1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] libvirt-domain.h: Fix enum description placement

Michal Privoznik posted 1 patch 6 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/f4cb85c6aff7c1d909115614d7d29ce7676789e6.1500634326.git.mprivozn@redhat.com
include/libvirt/libvirt-domain.h | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
[libvirt] [PATCH] libvirt-domain.h: Fix enum description placement
Posted by Michal Privoznik 6 years, 9 months ago 
There are only two acceptable places for describing enum values.
It's either:

    typedef enum {
        /* Some long description. Therefore it's placed before
         * the value. */
        VIR_ENUM_A_VAL = 1,
    } virEnumA;

or:

    typedef enum {
        VIR_ENUM_B_VAL = 1, /* Some short description */
    } virEnumB;

However, during review of a patch sent upstream I realized that
is not always the case. I went through all the public header
files and identified all the offenders. Luckily there were just
two of them.

Yes, this makes our HTML generated documentation broken, but
that's bug of the generator. Our header files shouldn't be forced
to use something we don't want to.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 include/libvirt/libvirt-domain.h | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 45f939a8c..a3bb9cbe9 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -2309,21 +2309,21 @@ int virDomainSetPerfEvents(virDomainPtr dom,
 typedef enum {
     VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN = 0, /* Placeholder */
 
-    VIR_DOMAIN_BLOCK_JOB_TYPE_PULL = 1,
     /* Block Pull (virDomainBlockPull, or virDomainBlockRebase without
      * flags), job ends on completion */
+    VIR_DOMAIN_BLOCK_JOB_TYPE_PULL = 1,
 
-    VIR_DOMAIN_BLOCK_JOB_TYPE_COPY = 2,
     /* Block Copy (virDomainBlockCopy, or virDomainBlockRebase with
      * flags), job exists as long as mirroring is active */
+    VIR_DOMAIN_BLOCK_JOB_TYPE_COPY = 2,
 
-    VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT = 3,
     /* Block Commit (virDomainBlockCommit without flags), job ends on
      * completion */
+    VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT = 3,
 
-    VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT = 4,
     /* Active Block Commit (virDomainBlockCommit with flags), job
      * exists as long as sync is active */
+    VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT = 4,
 
 # ifdef VIR_ENUM_SENTINELS
     VIR_DOMAIN_BLOCK_JOB_TYPE_LAST
@@ -3712,12 +3712,13 @@ typedef void (*virConnectDomainEventBlockJobCallback)(virConnectPtr conn,
  * The reason describing why this callback is called
  */
 typedef enum {
-    VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START = 0,
-    /* removable media changed to empty according to startup policy as source
+    /* Removable media changed to empty according to startup policy as source
      * was missing. oldSrcPath is set, newSrcPath is NULL */
-    VIR_DOMAIN_EVENT_DISK_DROP_MISSING_ON_START = 1,
-    /* disk was dropped from domain as source file was missing.
+    VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START = 0,
+
+    /* Disk was dropped from domain as source file was missing.
      * oldSrcPath is set, newSrcPath is NULL */
+    VIR_DOMAIN_EVENT_DISK_DROP_MISSING_ON_START = 1,
 
 # ifdef VIR_ENUM_SENTINELS
     VIR_DOMAIN_EVENT_DISK_CHANGE_LAST
-- 
2.13.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libvirt-domain.h: Fix enum description placement
Posted by Daniel P. Berrange 6 years, 9 months ago 
On Fri, Jul 21, 2017 at 12:52:06PM +0200, Michal Privoznik wrote:
> There are only two acceptable places for describing enum values.
> It's either:
> 
>     typedef enum {
>         /* Some long description. Therefore it's placed before
>          * the value. */
>         VIR_ENUM_A_VAL = 1,
>     } virEnumA;
> 
> or:
> 
>     typedef enum {
>         VIR_ENUM_B_VAL = 1, /* Some short description */
>     } virEnumB;
> 
> However, during review of a patch sent upstream I realized that
> is not always the case. I went through all the public header
> files and identified all the offenders. Luckily there were just
> two of them.
> 
> Yes, this makes our HTML generated documentation broken, but
> that's bug of the generator. Our header files shouldn't be forced
> to use something we don't want to.

FWIW, the problem is in the parseEnumBLock() method in apibuild.py

Once it has completed parsing an enum item, it delays adding that
enum to the list until it sees the next item, so that it can capture
the trailing comment.

The only way we can distinguish between a comment that comes before
the enum vs a comment after the enum, on the same line, is by detecting
whitespace (newline) before the trailing comment. Unfortunately I don't
thing this is exposed by the lexer right, so its not entirely easy
to solve.


> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  include/libvirt/libvirt-domain.h | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libvirt-domain.h: Fix enum description placement
Posted by Martin Kletzander 6 years, 9 months ago 
On Fri, Jul 21, 2017 at 11:58:45AM +0100, Daniel P. Berrange wrote:
>On Fri, Jul 21, 2017 at 12:52:06PM +0200, Michal Privoznik wrote:
>> There are only two acceptable places for describing enum values.
>> It's either:
>>
>>     typedef enum {
>>         /* Some long description. Therefore it's placed before
>>          * the value. */
>>         VIR_ENUM_A_VAL = 1,
>>     } virEnumA;
>>
>> or:
>>
>>     typedef enum {
>>         VIR_ENUM_B_VAL = 1, /* Some short description */
>>     } virEnumB;
>>
>> However, during review of a patch sent upstream I realized that
>> is not always the case. I went through all the public header
>> files and identified all the offenders. Luckily there were just
>> two of them.
>>
>> Yes, this makes our HTML generated documentation broken, but
>> that's bug of the generator. Our header files shouldn't be forced
>> to use something we don't want to.
>
>FWIW, the problem is in the parseEnumBLock() method in apibuild.py
>
>Once it has completed parsing an enum item, it delays adding that
>enum to the list until it sees the next item, so that it can capture
>the trailing comment.
>
>The only way we can distinguish between a comment that comes before
>the enum vs a comment after the enum, on the same line, is by detecting
>whitespace (newline) before the trailing comment. Unfortunately I don't
>thing this is exposed by the lexer right, so its not entirely easy
>to solve.
>

I was under the impression that this worked, we only broke it by some
recent commit.  I looked at the code and got pretty confused by it, but
shouldn't it be pretty easy (from a big picture view)?  You read until
you have both comment and a member of the struct.

If it's really harder than I think, then we can start using some helper
characters for the comments (at least for now) so that we can properly
identify them, e.g.:

struct meh {
       /*# This is comment for the following member foo */
       unsigned int foo;
       int bar; /*< This is for member bar that's on the same line */
}

and so on.  If that doesn't help either and it never worked,
then... it's a pity :-/

>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  include/libvirt/libvirt-domain.h | 17 +++++++++--------
>>  1 file changed, 9 insertions(+), 8 deletions(-)
>
>Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
>
>
>
>Regards,
>Daniel
>--
>|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libvirt-domain.h: Fix enum description placement
Posted by Daniel P. Berrange 6 years, 9 months ago 
On Fri, Jul 21, 2017 at 03:00:20PM +0200, Martin Kletzander wrote:
> On Fri, Jul 21, 2017 at 11:58:45AM +0100, Daniel P. Berrange wrote:
> > On Fri, Jul 21, 2017 at 12:52:06PM +0200, Michal Privoznik wrote:
> > > There are only two acceptable places for describing enum values.
> > > It's either:
> > > 
> > >     typedef enum {
> > >         /* Some long description. Therefore it's placed before
> > >          * the value. */
> > >         VIR_ENUM_A_VAL = 1,
> > >     } virEnumA;
> > > 
> > > or:
> > > 
> > >     typedef enum {
> > >         VIR_ENUM_B_VAL = 1, /* Some short description */
> > >     } virEnumB;
> > > 
> > > However, during review of a patch sent upstream I realized that
> > > is not always the case. I went through all the public header
> > > files and identified all the offenders. Luckily there were just
> > > two of them.
> > > 
> > > Yes, this makes our HTML generated documentation broken, but
> > > that's bug of the generator. Our header files shouldn't be forced
> > > to use something we don't want to.
> > 
> > FWIW, the problem is in the parseEnumBLock() method in apibuild.py
> > 
> > Once it has completed parsing an enum item, it delays adding that
> > enum to the list until it sees the next item, so that it can capture
> > the trailing comment.
> > 
> > The only way we can distinguish between a comment that comes before
> > the enum vs a comment after the enum, on the same line, is by detecting
> > whitespace (newline) before the trailing comment. Unfortunately I don't
> > thing this is exposed by the lexer right, so its not entirely easy
> > to solve.
> > 
> 
> I was under the impression that this worked, we only broke it by some
> recent commit.  I looked at the code and got pretty confused by it, but
> shouldn't it be pretty easy (from a big picture view)?  You read until
> you have both comment and a member of the struct.
> 
> If it's really harder than I think, then we can start using some helper
> characters for the comments (at least for now) so that we can properly
> identify them, e.g.:
> 
> struct meh {
>       /*# This is comment for the following member foo */
>       unsigned int foo;
>       int bar; /*< This is for member bar that's on the same line */
> }
> 
> and so on.  If that doesn't help either and it never worked,
> then... it's a pity :-/

That is ambiguous - without seeing whitespace, the parser cannot
distinguish between these two scenarios:

 struct meh {
       unsigned int foo; /*# This is comment for the following member foo */
       int bar;
 }

 struct meh {
       unsigned int foo;
       
        /*# This is comment for the following member foo */
       int bar;
 }




Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libvirt-domain.h: Fix enum description placement
Posted by Jiri Denemark 6 years, 9 months ago 
On Fri, Jul 21, 2017 at 14:42:05 +0100, Daniel P. Berrange wrote:
> On Fri, Jul 21, 2017 at 03:00:20PM +0200, Martin Kletzander wrote:

> > struct meh {
> >       /*# This is comment for the following member foo */
> >       unsigned int foo;
> >       int bar; /*< This is for member bar that's on the same line */
> > }
> > 
> > and so on.  If that doesn't help either and it never worked,
> > then... it's a pity :-/
> 
> That is ambiguous - without seeing whitespace, the parser cannot
> distinguish between these two scenarios:
> 
>  struct meh {
>        unsigned int foo; /*# This is comment for the following member foo */
>        int bar;
>  }

Martin suggested < to be used instead of # for this type of comments to
remove the ambiguity.

>  struct meh {
>        unsigned int foo;
>        
>         /*# This is comment for the following member foo */
>        int bar;
>  }

However, I think we could just simply force using only comments above
each member in our public header files and fix the generator to properly
work with them. This should be a lot easier than fixing it to handle
both options.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libvirt-domain.h: Fix enum description placement
Posted by Daniel P. Berrange 6 years, 9 months ago 
On Fri, Jul 21, 2017 at 04:33:06PM +0200, Jiri Denemark wrote:
> On Fri, Jul 21, 2017 at 14:42:05 +0100, Daniel P. Berrange wrote:
> > On Fri, Jul 21, 2017 at 03:00:20PM +0200, Martin Kletzander wrote:
> 
> > > struct meh {
> > >       /*# This is comment for the following member foo */
> > >       unsigned int foo;
> > >       int bar; /*< This is for member bar that's on the same line */
> > > }
> > > 
> > > and so on.  If that doesn't help either and it never worked,
> > > then... it's a pity :-/
> > 
> > That is ambiguous - without seeing whitespace, the parser cannot
> > distinguish between these two scenarios:
> > 
> >  struct meh {
> >        unsigned int foo; /*# This is comment for the following member foo */
> >        int bar;
> >  }
> 
> Martin suggested < to be used instead of # for this type of comments to
> remove the ambiguity.
> 
> >  struct meh {
> >        unsigned int foo;
> >        
> >         /*# This is comment for the following member foo */
> >        int bar;
> >  }
> 
> However, I think we could just simply force using only comments above
> each member in our public header files and fix the generator to properly
> work with them. This should be a lot easier than fixing it to handle
> both options.

I'd probably go for doing that. In many cases where we put the comment
at the end of the line, we're forced to split it across many lines
anyway, lest the line get too long. Putting commments before the
item means we won't hit the line length so easily, so more comments
will fit on one line.

Oh and this issue affects struct field members too IIUC

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libvirt-domain.h: Fix enum description placement
Posted by Martin Kletzander 6 years, 9 months ago 
On Fri, Jul 21, 2017 at 02:42:05PM +0100, Daniel P. Berrange wrote:
>On Fri, Jul 21, 2017 at 03:00:20PM +0200, Martin Kletzander wrote:
>> On Fri, Jul 21, 2017 at 11:58:45AM +0100, Daniel P. Berrange wrote:
>> > On Fri, Jul 21, 2017 at 12:52:06PM +0200, Michal Privoznik wrote:
>> > > There are only two acceptable places for describing enum values.
>> > > It's either:
>> > >
>> > >     typedef enum {
>> > >         /* Some long description. Therefore it's placed before
>> > >          * the value. */
>> > >         VIR_ENUM_A_VAL = 1,
>> > >     } virEnumA;
>> > >
>> > > or:
>> > >
>> > >     typedef enum {
>> > >         VIR_ENUM_B_VAL = 1, /* Some short description */
>> > >     } virEnumB;
>> > >
>> > > However, during review of a patch sent upstream I realized that
>> > > is not always the case. I went through all the public header
>> > > files and identified all the offenders. Luckily there were just
>> > > two of them.
>> > >
>> > > Yes, this makes our HTML generated documentation broken, but
>> > > that's bug of the generator. Our header files shouldn't be forced
>> > > to use something we don't want to.
>> >
>> > FWIW, the problem is in the parseEnumBLock() method in apibuild.py
>> >
>> > Once it has completed parsing an enum item, it delays adding that
>> > enum to the list until it sees the next item, so that it can capture
>> > the trailing comment.
>> >
>> > The only way we can distinguish between a comment that comes before
>> > the enum vs a comment after the enum, on the same line, is by detecting
>> > whitespace (newline) before the trailing comment. Unfortunately I don't
>> > thing this is exposed by the lexer right, so its not entirely easy
>> > to solve.
>> >
>>
>> I was under the impression that this worked, we only broke it by some
>> recent commit.  I looked at the code and got pretty confused by it, but
>> shouldn't it be pretty easy (from a big picture view)?  You read until
>> you have both comment and a member of the struct.
>>
>> If it's really harder than I think, then we can start using some helper
>> characters for the comments (at least for now) so that we can properly
>> identify them, e.g.:
>>
>> struct meh {
>>       /*# This is comment for the following member foo */
>>       unsigned int foo;
>>       int bar; /*< This is for member bar that's on the same line */
>> }
>>
>> and so on.  If that doesn't help either and it never worked,
>> then... it's a pity :-/
>
>That is ambiguous - without seeing whitespace, the parser cannot
>distinguish between these two scenarios:
>

Oh, you're right, so the only other easy workaround option that I can
think of is to just document every single thing, then the parser can
always just first two things (comment and member) together.

Or, as Jirka suggested, just make it mandatory to put the comments
before and fix the script to accept just that order.

> struct meh {
>       unsigned int foo; /*# This is comment for the following member foo */
>       int bar;
> }
>
> struct meh {
>       unsigned int foo;
>
>        /*# This is comment for the following member foo */
>       int bar;
> }
>
>
>
>
>Regards,
>Daniel
>--
>|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.