[libvirt] [PATCH] libvirt-domain.h: Fix enum description placement
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
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 :|
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
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 :|
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
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 :|
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
participants (4)
-
Daniel P. Berrange -
Jiri Denemark -
Martin Kletzander -
Michal Privoznik