Due to hystorical reasons, it needs to be possible to pass values
from the virTypedParameterFlags and virDomainModificationImpact
enumerations to a function at the same time, so it is very
important that the two never overlap.
Right now this is "enforced" by the presence of special comments;
unfortunately, said comments are not handled correctly by
apibuild.py and end up, quite confusingly, showing up as part of
the documentation for symbols preceding or following them.
Introduce actual entires in each enumeration for each of the
overlapping values, which is more explicit and results in
comments being parsed correctly.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
include/libvirt/libvirt-common.h.in | 19 +++++++++++++++++--
include/libvirt/libvirt-domain.h | 8 ++++----
2 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/include/libvirt/libvirt-common.h.in b/include/libvirt/libvirt-common.h.in
index ccdbb2a100..2f20456dfd 100644
--- a/include/libvirt/libvirt-common.h.in
+++ b/include/libvirt/libvirt-common.h.in
@@ -159,8 +159,23 @@ typedef enum {
* Since: 0.9.8
*/
typedef enum {
- /* 1 << 0 is reserved for virDomainModificationImpact */
- /* 1 << 1 is reserved for virDomainModificationImpact */
+ /* Reserved for virDomainModificationImpact. Do not use.
+ *
+ * Since: 8.4.0
+ */
+ VIR_TYPED_PARAM_RESERVED1 = 0,
+
+ /* Reserved for virDomainModificationImpact. Do not use.
+ *
+ * Since: 8.4.0
+ */
+ VIR_TYPED_PARAM_RESERVED2 = 1 << 0,
+
+ /* Reserved for virDomainModificationImpact. Do not use.
+ *
+ * Since: 8.4.0
+ */
+ VIR_TYPED_PARAM_RESERVED3 = 1 << 1,
/* Older servers lacked the ability to handle string typed
* parameters. Attempts to set a string parameter with an older
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 2edef9c4e1..94cb4a6615 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -321,10 +321,10 @@ typedef virDomainControlInfo *virDomainControlInfoPtr;
* Since: 0.9.2
*/
typedef enum {
- VIR_DOMAIN_AFFECT_CURRENT = 0, /* Affect current domain state. (Since: 0.9.2) */
- VIR_DOMAIN_AFFECT_LIVE = 1 << 0, /* Affect running domain state. (Since: 0.9.2) */
- VIR_DOMAIN_AFFECT_CONFIG = 1 << 1, /* Affect persistent domain state. (Since: 0.9.2) */
- /* 1 << 2 is reserved for virTypedParameterFlags */
+ VIR_DOMAIN_AFFECT_CURRENT = 0, /* Affect current domain state. (Since: 0.9.2) */
+ VIR_DOMAIN_AFFECT_LIVE = 1 << 0, /* Affect running domain state. (Since: 0.9.2) */
+ VIR_DOMAIN_AFFECT_CONFIG = 1 << 1, /* Affect persistent domain state. (Since: 0.9.2) */
+ VIR_DOMAIN_AFFECT_RESERVED1 = 1 << 2, /* Reserved for virTypedParameterFlags. Do not use. (Since: 8.4.0) */
} virDomainModificationImpact;
/**
--
2.35.1On Wed, May 04, 2022 at 07:30:08PM +0200, Andrea Bolognani wrote:
> Due to hystorical reasons, it needs to be possible to pass values
> from the virTypedParameterFlags and virDomainModificationImpact
> enumerations to a function at the same time, so it is very
> important that the two never overlap.
>
> Right now this is "enforced" by the presence of special comments;
> unfortunately, said comments are not handled correctly by
> apibuild.py and end up, quite confusingly, showing up as part of
> the documentation for symbols preceding or following them.
>
> Introduce actual entires in each enumeration for each of the
> overlapping values, which is more explicit and results in
> comments being parsed correctly.
I don't really like the idea of adding stuff to the public API
to workaround brokenness in apibuild.py.
It seems like we only need apibuild.py to not merge together
distinct comment blocks.
>
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
> include/libvirt/libvirt-common.h.in | 19 +++++++++++++++++--
> include/libvirt/libvirt-domain.h | 8 ++++----
> 2 files changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/include/libvirt/libvirt-common.h.in b/include/libvirt/libvirt-common.h.in
> index ccdbb2a100..2f20456dfd 100644
> --- a/include/libvirt/libvirt-common.h.in
> +++ b/include/libvirt/libvirt-common.h.in
> @@ -159,8 +159,23 @@ typedef enum {
> * Since: 0.9.8
> */
> typedef enum {
> - /* 1 << 0 is reserved for virDomainModificationImpact */
> - /* 1 << 1 is reserved for virDomainModificationImpact */
> + /* Reserved for virDomainModificationImpact. Do not use.
> + *
> + * Since: 8.4.0
> + */
> + VIR_TYPED_PARAM_RESERVED1 = 0,
> +
> + /* Reserved for virDomainModificationImpact. Do not use.
> + *
> + * Since: 8.4.0
> + */
> + VIR_TYPED_PARAM_RESERVED2 = 1 << 0,
> +
> + /* Reserved for virDomainModificationImpact. Do not use.
> + *
> + * Since: 8.4.0
> + */
> + VIR_TYPED_PARAM_RESERVED3 = 1 << 1,
>
> /* Older servers lacked the ability to handle string typed
> * parameters. Attempts to set a string parameter with an older
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index 2edef9c4e1..94cb4a6615 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -321,10 +321,10 @@ typedef virDomainControlInfo *virDomainControlInfoPtr;
> * Since: 0.9.2
> */
> typedef enum {
> - VIR_DOMAIN_AFFECT_CURRENT = 0, /* Affect current domain state. (Since: 0.9.2) */
> - VIR_DOMAIN_AFFECT_LIVE = 1 << 0, /* Affect running domain state. (Since: 0.9.2) */
> - VIR_DOMAIN_AFFECT_CONFIG = 1 << 1, /* Affect persistent domain state. (Since: 0.9.2) */
> - /* 1 << 2 is reserved for virTypedParameterFlags */
> + VIR_DOMAIN_AFFECT_CURRENT = 0, /* Affect current domain state. (Since: 0.9.2) */
> + VIR_DOMAIN_AFFECT_LIVE = 1 << 0, /* Affect running domain state. (Since: 0.9.2) */
> + VIR_DOMAIN_AFFECT_CONFIG = 1 << 1, /* Affect persistent domain state. (Since: 0.9.2) */
> + VIR_DOMAIN_AFFECT_RESERVED1 = 1 << 2, /* Reserved for virTypedParameterFlags. Do not use. (Since: 8.4.0) */
> } virDomainModificationImpact;
>
> /**
> --
> 2.35.1
>
With 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 :|
Hi,
On Wed, May 04, 2022 at 07:02:48PM +0100, Daniel P. Berrangé wrote:
> On Wed, May 04, 2022 at 07:30:08PM +0200, Andrea Bolognani wrote:
> > Due to hystorical reasons, it needs to be possible to pass values
hystorical -> historical ?
> > from the virTypedParameterFlags and virDomainModificationImpact
> > enumerations to a function at the same time, so it is very
> > important that the two never overlap.
> >
> > Right now this is "enforced" by the presence of special comments;
> > unfortunately, said comments are not handled correctly by
> > apibuild.py and end up, quite confusingly, showing up as part of
> > the documentation for symbols preceding or following them.
> >
> > Introduce actual entires in each enumeration for each of the
> > overlapping values, which is more explicit and results in
> > comments being parsed correctly.
>
> I don't really like the idea of adding stuff to the public API
> to workaround brokenness in apibuild.py.
While apibuild.py needs to be fixed to error/warn in this
scenarios, I'd argue that the patch moves towards consistency
with comments blocks and improves the documentation of already
exposed API.
> It seems like we only need apibuild.py to not merge together
> distinct comment blocks.
What is not trivial is to (1) define which comment block belongs
to which element/type. We need to define what is acceptable and
what is not and (2) enforce that to stay consistent.
> > Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> > ---
> > include/libvirt/libvirt-common.h.in | 19 +++++++++++++++++--
> > include/libvirt/libvirt-domain.h | 8 ++++----
> > 2 files changed, 21 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/libvirt/libvirt-common.h.in b/include/libvirt/libvirt-common.h.in
> > index ccdbb2a100..2f20456dfd 100644
> > --- a/include/libvirt/libvirt-common.h.in
> > +++ b/include/libvirt/libvirt-common.h.in
> > @@ -159,8 +159,23 @@ typedef enum {
> > * Since: 0.9.8
> > */
> > typedef enum {
> > - /* 1 << 0 is reserved for virDomainModificationImpact */
> > - /* 1 << 1 is reserved for virDomainModificationImpact */
> > + /* Reserved for virDomainModificationImpact. Do not use.
> > + *
> > + * Since: 8.4.0
> > + */
> > + VIR_TYPED_PARAM_RESERVED1 = 0,
> > +
> > + /* Reserved for virDomainModificationImpact. Do not use.
> > + *
> > + * Since: 8.4.0
> > + */
> > + VIR_TYPED_PARAM_RESERVED2 = 1 << 0,
> > +
> > + /* Reserved for virDomainModificationImpact. Do not use.
> > + *
> > + * Since: 8.4.0
> > + */
> > + VIR_TYPED_PARAM_RESERVED3 = 1 << 1,
> >
> > /* Older servers lacked the ability to handle string typed
> > * parameters. Attempts to set a string parameter with an older
> > diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> > index 2edef9c4e1..94cb4a6615 100644
> > --- a/include/libvirt/libvirt-domain.h
> > +++ b/include/libvirt/libvirt-domain.h
> > @@ -321,10 +321,10 @@ typedef virDomainControlInfo *virDomainControlInfoPtr;
> > * Since: 0.9.2
> > */
> > typedef enum {
> > - VIR_DOMAIN_AFFECT_CURRENT = 0, /* Affect current domain state. (Since: 0.9.2) */
> > - VIR_DOMAIN_AFFECT_LIVE = 1 << 0, /* Affect running domain state. (Since: 0.9.2) */
> > - VIR_DOMAIN_AFFECT_CONFIG = 1 << 1, /* Affect persistent domain state. (Since: 0.9.2) */
> > - /* 1 << 2 is reserved for virTypedParameterFlags */
> > + VIR_DOMAIN_AFFECT_CURRENT = 0, /* Affect current domain state. (Since: 0.9.2) */
> > + VIR_DOMAIN_AFFECT_LIVE = 1 << 0, /* Affect running domain state. (Since: 0.9.2) */
> > + VIR_DOMAIN_AFFECT_CONFIG = 1 << 1, /* Affect persistent domain state. (Since: 0.9.2) */
> > + VIR_DOMAIN_AFFECT_RESERVED1 = 1 << 2, /* Reserved for virTypedParameterFlags. Do not use. (Since: 8.4.0) */
> > } virDomainModificationImpact;
> >
> > /**
> > --
> > 2.35.1
> >
>
> With regards,
> Daniel
Cheers,
Victor
On Thu, May 05, 2022 at 09:48:54AM +0200, Victor Toso wrote:
> Hi,
>
> On Wed, May 04, 2022 at 07:02:48PM +0100, Daniel P. Berrangé wrote:
> > On Wed, May 04, 2022 at 07:30:08PM +0200, Andrea Bolognani wrote:
> > > Due to hystorical reasons, it needs to be possible to pass values
>
> hystorical -> historical ?
>
> > > from the virTypedParameterFlags and virDomainModificationImpact
> > > enumerations to a function at the same time, so it is very
> > > important that the two never overlap.
> > >
> > > Right now this is "enforced" by the presence of special comments;
> > > unfortunately, said comments are not handled correctly by
> > > apibuild.py and end up, quite confusingly, showing up as part of
> > > the documentation for symbols preceding or following them.
> > >
> > > Introduce actual entires in each enumeration for each of the
> > > overlapping values, which is more explicit and results in
> > > comments being parsed correctly.
> >
> > I don't really like the idea of adding stuff to the public API
> > to workaround brokenness in apibuild.py.
>
> While apibuild.py needs to be fixed to error/warn in this
> scenarios, I'd argue that the patch moves towards consistency
> with comments blocks and improves the documentation of already
> exposed API.
>
> > It seems like we only need apibuild.py to not merge together
> > distinct comment blocks.
>
> What is not trivial is to (1) define which comment block belongs
> to which element/type. We need to define what is acceptable and
> what is not and (2) enforce that to stay consistent.
If we have multiple opened+clsoed comment blocks immediately after
each other such as this scenario:
/* 1 << 0 is reserved for virDomainModificationImpact */
/* 1 << 1 is reserved for virDomainModificationImpact */
/* Older servers lacked the ability to handle string typed
* parameters. Attempts to set a string parameter with an older
* server will fail at the client, but attempts to retrieve
* parameters must not return strings from a new server to an
* older client, so this flag exists to identify newer clients to
* newer servers. This flag is automatically set when needed, so
* the user does not have to worry about it; however, manually
* setting the flag can be used to reject servers that cannot
* return typed strings, even if no strings would be returned.
*
* Since: v0.9.8
*/
VIR_TYPED_PARAM_STRING_OKAY = 1 << 2,
IMHO it is pretty straightforward for apibuild.py to have a policy
that the comment block closest to the declaration is the API docs
and the preceeding ones are irrelevant to hte API docs.
I very much doubt we hav a case where we have multiple open+closed
comment blocks which all should be part of the API docs for a given
declaration, and if we did, then we should merge them into a single
open+closed comment block.
>
> > > Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> > > ---
> > > include/libvirt/libvirt-common.h.in | 19 +++++++++++++++++--
> > > include/libvirt/libvirt-domain.h | 8 ++++----
> > > 2 files changed, 21 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/include/libvirt/libvirt-common.h.in b/include/libvirt/libvirt-common.h.in
> > > index ccdbb2a100..2f20456dfd 100644
> > > --- a/include/libvirt/libvirt-common.h.in
> > > +++ b/include/libvirt/libvirt-common.h.in
> > > @@ -159,8 +159,23 @@ typedef enum {
> > > * Since: 0.9.8
> > > */
> > > typedef enum {
> > > - /* 1 << 0 is reserved for virDomainModificationImpact */
> > > - /* 1 << 1 is reserved for virDomainModificationImpact */
> > > + /* Reserved for virDomainModificationImpact. Do not use.
> > > + *
> > > + * Since: 8.4.0
> > > + */
> > > + VIR_TYPED_PARAM_RESERVED1 = 0,
> > > +
> > > + /* Reserved for virDomainModificationImpact. Do not use.
> > > + *
> > > + * Since: 8.4.0
> > > + */
> > > + VIR_TYPED_PARAM_RESERVED2 = 1 << 0,
> > > +
> > > + /* Reserved for virDomainModificationImpact. Do not use.
> > > + *
> > > + * Since: 8.4.0
> > > + */
> > > + VIR_TYPED_PARAM_RESERVED3 = 1 << 1,
> > >
> > > /* Older servers lacked the ability to handle string typed
> > > * parameters. Attempts to set a string parameter with an older
> > > diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> > > index 2edef9c4e1..94cb4a6615 100644
> > > --- a/include/libvirt/libvirt-domain.h
> > > +++ b/include/libvirt/libvirt-domain.h
> > > @@ -321,10 +321,10 @@ typedef virDomainControlInfo *virDomainControlInfoPtr;
> > > * Since: 0.9.2
> > > */
> > > typedef enum {
> > > - VIR_DOMAIN_AFFECT_CURRENT = 0, /* Affect current domain state. (Since: 0.9.2) */
> > > - VIR_DOMAIN_AFFECT_LIVE = 1 << 0, /* Affect running domain state. (Since: 0.9.2) */
> > > - VIR_DOMAIN_AFFECT_CONFIG = 1 << 1, /* Affect persistent domain state. (Since: 0.9.2) */
> > > - /* 1 << 2 is reserved for virTypedParameterFlags */
> > > + VIR_DOMAIN_AFFECT_CURRENT = 0, /* Affect current domain state. (Since: 0.9.2) */
> > > + VIR_DOMAIN_AFFECT_LIVE = 1 << 0, /* Affect running domain state. (Since: 0.9.2) */
> > > + VIR_DOMAIN_AFFECT_CONFIG = 1 << 1, /* Affect persistent domain state. (Since: 0.9.2) */
> > > + VIR_DOMAIN_AFFECT_RESERVED1 = 1 << 2, /* Reserved for virTypedParameterFlags. Do not use. (Since: 8.4.0) */
> > > } virDomainModificationImpact;
> > >
> > > /**
> > > --
> > > 2.35.1
> > >
> >
> > With regards,
> > Daniel
>
> Cheers,
> Victor
With 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 Thu, May 05, 2022 at 08:57:04AM +0100, Daniel P. Berrangé wrote: > On Thu, May 05, 2022 at 09:48:54AM +0200, Victor Toso wrote: > > On Wed, May 04, 2022 at 07:02:48PM +0100, Daniel P. Berrangé wrote: > > > I don't really like the idea of adding stuff to the public API > > > to workaround brokenness in apibuild.py. > > > > While apibuild.py needs to be fixed to error/warn in this > > scenarios, I'd argue that the patch moves towards consistency > > with comments blocks and improves the documentation of already > > exposed API. > > > > > It seems like we only need apibuild.py to not merge together > > > distinct comment blocks. > > > > What is not trivial is to (1) define which comment block belongs > > to which element/type. We need to define what is acceptable and > > what is not and (2) enforce that to stay consistent. > > If we have multiple opened+clsoed comment blocks immediately after > each other such as this scenario: > > /* 1 << 0 is reserved for virDomainModificationImpact */ > /* 1 << 1 is reserved for virDomainModificationImpact */ > > /* Older servers lacked the ability to handle string typed > * parameters. Attempts to set a string parameter with an older > * server will fail at the client, but attempts to retrieve > * parameters must not return strings from a new server to an > * older client, so this flag exists to identify newer clients to > * newer servers. This flag is automatically set when needed, so > * the user does not have to worry about it; however, manually > * setting the flag can be used to reject servers that cannot > * return typed strings, even if no strings would be returned. > * > * Since: v0.9.8 > */ > VIR_TYPED_PARAM_STRING_OKAY = 1 << 2, > > IMHO it is pretty straightforward for apibuild.py to have a policy > that the comment block closest to the declaration is the API docs > and the preceeding ones are irrelevant to hte API docs. > > I very much doubt we hav a case where we have multiple open+closed > comment blocks which all should be part of the API docs for a given > declaration, and if we did, then we should merge them into a single > open+closed comment block. This makes sense, at least in theory. I have no idea how difficult it would be to actually convince apibuild.py to behave this way though. I can give it a shot, but I'm concerned about falling into a real rabbit hole with this one. If I don't manage to bend the script to my will quickly enough, I'll just give up on the idea and leave things as they are. -- Andrea Bolognani / Red Hat / Virtualization
On Thu, May 05, 2022 at 02:13:15AM -0700, Andrea Bolognani wrote: > On Thu, May 05, 2022 at 08:57:04AM +0100, Daniel P. Berrangé wrote: > > On Thu, May 05, 2022 at 09:48:54AM +0200, Victor Toso wrote: > > > On Wed, May 04, 2022 at 07:02:48PM +0100, Daniel P. Berrangé wrote: > > > > I don't really like the idea of adding stuff to the public API > > > > to workaround brokenness in apibuild.py. > > > > > > While apibuild.py needs to be fixed to error/warn in this > > > scenarios, I'd argue that the patch moves towards consistency > > > with comments blocks and improves the documentation of already > > > exposed API. > > > > > > > It seems like we only need apibuild.py to not merge together > > > > distinct comment blocks. > > > > > > What is not trivial is to (1) define which comment block belongs > > > to which element/type. We need to define what is acceptable and > > > what is not and (2) enforce that to stay consistent. > > > > If we have multiple opened+clsoed comment blocks immediately after > > each other such as this scenario: > > > > /* 1 << 0 is reserved for virDomainModificationImpact */ > > /* 1 << 1 is reserved for virDomainModificationImpact */ > > > > /* Older servers lacked the ability to handle string typed > > * parameters. Attempts to set a string parameter with an older > > * server will fail at the client, but attempts to retrieve > > * parameters must not return strings from a new server to an > > * older client, so this flag exists to identify newer clients to > > * newer servers. This flag is automatically set when needed, so > > * the user does not have to worry about it; however, manually > > * setting the flag can be used to reject servers that cannot > > * return typed strings, even if no strings would be returned. > > * > > * Since: v0.9.8 > > */ > > VIR_TYPED_PARAM_STRING_OKAY = 1 << 2, > > > > IMHO it is pretty straightforward for apibuild.py to have a policy > > that the comment block closest to the declaration is the API docs > > and the preceeding ones are irrelevant to hte API docs. > > > > I very much doubt we hav a case where we have multiple open+closed > > comment blocks which all should be part of the API docs for a given > > declaration, and if we did, then we should merge them into a single > > open+closed comment block. > > This makes sense, at least in theory. I have no idea how difficult it > would be to actually convince apibuild.py to behave this way though. > > I can give it a shot, but I'm concerned about falling into a real > rabbit hole with this one. If I don't manage to bend the script to my > will quickly enough, I'll just give up on the idea and leave things > as they are. Alternatively the classic approach to this problem taken by javadoc and gtk-doc, is to require API comments to use '/**' and leave '/*' for non-API comments. We've got a reasonably large amount of usage of '/**' but I'm guessing it isn't complete. With 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 Thu, May 05, 2022 at 11:40:55AM +0100, Daniel P. Berrangé wrote: > On Thu, May 05, 2022 at 02:13:15AM -0700, Andrea Bolognani wrote: > > On Thu, May 05, 2022 at 08:57:04AM +0100, Daniel P. Berrangé wrote: > > > IMHO it is pretty straightforward for apibuild.py to have a policy > > > that the comment block closest to the declaration is the API docs > > > and the preceeding ones are irrelevant to hte API docs. > > > > > > I very much doubt we hav a case where we have multiple open+closed > > > comment blocks which all should be part of the API docs for a given > > > declaration, and if we did, then we should merge them into a single > > > open+closed comment block. > > > > This makes sense, at least in theory. I have no idea how difficult it > > would be to actually convince apibuild.py to behave this way though. > > > > I can give it a shot, but I'm concerned about falling into a real > > rabbit hole with this one. If I don't manage to bend the script to my > > will quickly enough, I'll just give up on the idea and leave things > > as they are. > > Alternatively the classic approach to this problem taken by javadoc > and gtk-doc, is to require API comments to use '/**' and leave '/*' > for non-API comments. We've got a reasonably large amount of usage > of '/**' but I'm guessing it isn't complete. Yeah I wouldn't mind if we got rid of same-line comments altogether and used block comments everywhere. We wouldn't necessarily even have to change apibuild.py, just update all existing uses. That'd result in a lot of churn, of course. Do you have an opinion on the possibility of switching to an off-the-shelf solution for documenting our API? I name-dropped Doxygen in the past, but I'm not actually familiar with it. Not sure how gtk-doc would work for a library that doesn't expose a GLib-based API. -- Andrea Bolognani / Red Hat / Virtualization
On Thu, May 05, 2022 at 04:58:10AM -0700, Andrea Bolognani wrote: > On Thu, May 05, 2022 at 11:40:55AM +0100, Daniel P. Berrangé wrote: > > On Thu, May 05, 2022 at 02:13:15AM -0700, Andrea Bolognani wrote: > > > On Thu, May 05, 2022 at 08:57:04AM +0100, Daniel P. Berrangé wrote: > > > > IMHO it is pretty straightforward for apibuild.py to have a policy > > > > that the comment block closest to the declaration is the API docs > > > > and the preceeding ones are irrelevant to hte API docs. > > > > > > > > I very much doubt we hav a case where we have multiple open+closed > > > > comment blocks which all should be part of the API docs for a given > > > > declaration, and if we did, then we should merge them into a single > > > > open+closed comment block. > > > > > > This makes sense, at least in theory. I have no idea how difficult it > > > would be to actually convince apibuild.py to behave this way though. > > > > > > I can give it a shot, but I'm concerned about falling into a real > > > rabbit hole with this one. If I don't manage to bend the script to my > > > will quickly enough, I'll just give up on the idea and leave things > > > as they are. > > > > Alternatively the classic approach to this problem taken by javadoc > > and gtk-doc, is to require API comments to use '/**' and leave '/*' > > for non-API comments. We've got a reasonably large amount of usage > > of '/**' but I'm guessing it isn't complete. > > Yeah I wouldn't mind if we got rid of same-line comments altogether > and used block comments everywhere. We wouldn't necessarily even have > to change apibuild.py, just update all existing uses. That'd result > in a lot of churn, of course. > > Do you have an opinion on the possibility of switching to an > off-the-shelf solution for documenting our API? I name-dropped > Doxygen in the past, but I'm not actually familiar with it. Not sure > how gtk-doc would work for a library that doesn't expose a GLib-based > API. I'm filled with despair any time I look at a project whose API docs are generated by Doxygen. All the information is there, but somehow Doxygen manages to make it awful to find the things you want. To be fair I think is largely because C is unimaginably flexible. gtk-doc produces much nicer docs, but it is slightly opinionated about the way your API is structured and naming conventions in use, so won't work for all C libraries. It has support for GObject specific features but you can ignore that. It'd probably be able to make it work for libvirt with some hacking. Unfortunately gtk-doc is being pushed towards retirement: https://blog.gtk.org/2021/08/19/the-gtk-documentation/ while the code isn't going anywhere, it wont likely get much love going fowards. The replacement based on gobject introspection is likely to be quite challenging to use for libvirt, as it is a bit more opinionated about API design than even gtk-doc. On the flip side if it were possible to get gobject introspection working with libvirt, it'd open up ability to use libvirt from many languages without manually writing bindings. Finally, even if we did use something else for API docs, we still actually need apibuild.py to work correctly, because we use it to expose an XML representation of our API that several language bindings rely on. 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 :|
Hi,
On Thu, May 05, 2022 at 08:57:04AM +0100, Daniel P. Berrangé wrote:
> On Thu, May 05, 2022 at 09:48:54AM +0200, Victor Toso wrote:
> > Hi,
> >
> > On Wed, May 04, 2022 at 07:02:48PM +0100, Daniel P. Berrangé wrote:
> > > On Wed, May 04, 2022 at 07:30:08PM +0200, Andrea Bolognani wrote:
> > > > Due to hystorical reasons, it needs to be possible to pass values
> >
> > hystorical -> historical ?
> >
> > > > from the virTypedParameterFlags and virDomainModificationImpact
> > > > enumerations to a function at the same time, so it is very
> > > > important that the two never overlap.
> > > >
> > > > Right now this is "enforced" by the presence of special comments;
> > > > unfortunately, said comments are not handled correctly by
> > > > apibuild.py and end up, quite confusingly, showing up as part of
> > > > the documentation for symbols preceding or following them.
> > > >
> > > > Introduce actual entires in each enumeration for each of the
> > > > overlapping values, which is more explicit and results in
> > > > comments being parsed correctly.
> > >
> > > I don't really like the idea of adding stuff to the public API
> > > to workaround brokenness in apibuild.py.
> >
> > While apibuild.py needs to be fixed to error/warn in this
> > scenarios, I'd argue that the patch moves towards consistency
> > with comments blocks and improves the documentation of already
> > exposed API.
> >
> > > It seems like we only need apibuild.py to not merge together
> > > distinct comment blocks.
> >
> > What is not trivial is to (1) define which comment block belongs
> > to which element/type. We need to define what is acceptable and
> > what is not and (2) enforce that to stay consistent.
>
> If we have multiple opened+clsoed comment blocks immediately after
> each other such as this scenario:
>
> /* 1 << 0 is reserved for virDomainModificationImpact */
> /* 1 << 1 is reserved for virDomainModificationImpact */
>
> /* Older servers lacked the ability to handle string typed
> * parameters. Attempts to set a string parameter with an older
> * server will fail at the client, but attempts to retrieve
> * parameters must not return strings from a new server to an
> * older client, so this flag exists to identify newer clients to
> * newer servers. This flag is automatically set when needed, so
> * the user does not have to worry about it; however, manually
> * setting the flag can be used to reject servers that cannot
> * return typed strings, even if no strings would be returned.
> *
> * Since: v0.9.8
> */
> VIR_TYPED_PARAM_STRING_OKAY = 1 << 2,
>
>
> IMHO it is pretty straightforward for apibuild.py to have a policy
> that the comment block closest to the declaration is the API docs
> and the preceeding ones are irrelevant to hte API docs.
While working on the Since metadata, I recall finding instances
of:
/* (C1) Comment before enum value */
ENUM_VALUE, /* (C2) Comment same line
that might span to multiple lines */
/* (C3) Comment after enum value */
Not necessary at the same time, but the point is that they were
meant to document ENUM_VALUE. IIRC, (C3) was only for the enum
sentinel value (_ENUM_TYPE_LAST).
Sure, we can pick one and follow it but there is some fixing to
do in the existing documentation to keep it consistent.
> I very much doubt we hav a case where we have multiple open+closed
> comment blocks which all should be part of the API docs for a given
> declaration, and if we did, then we should merge them into a single
> open+closed comment block.
I think we should not merge them, but apibuild should warn/error
instead as it seems unlikely that two block of comments refer to
a single type.
> > > > Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> > > > ---
> > > > include/libvirt/libvirt-common.h.in | 19 +++++++++++++++++--
> > > > include/libvirt/libvirt-domain.h | 8 ++++----
> > > > 2 files changed, 21 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/include/libvirt/libvirt-common.h.in b/include/libvirt/libvirt-common.h.in
> > > > index ccdbb2a100..2f20456dfd 100644
> > > > --- a/include/libvirt/libvirt-common.h.in
> > > > +++ b/include/libvirt/libvirt-common.h.in
> > > > @@ -159,8 +159,23 @@ typedef enum {
> > > > * Since: 0.9.8
> > > > */
> > > > typedef enum {
> > > > - /* 1 << 0 is reserved for virDomainModificationImpact */
> > > > - /* 1 << 1 is reserved for virDomainModificationImpact */
> > > > + /* Reserved for virDomainModificationImpact. Do not use.
> > > > + *
> > > > + * Since: 8.4.0
> > > > + */
> > > > + VIR_TYPED_PARAM_RESERVED1 = 0,
> > > > +
> > > > + /* Reserved for virDomainModificationImpact. Do not use.
> > > > + *
> > > > + * Since: 8.4.0
> > > > + */
> > > > + VIR_TYPED_PARAM_RESERVED2 = 1 << 0,
> > > > +
> > > > + /* Reserved for virDomainModificationImpact. Do not use.
> > > > + *
> > > > + * Since: 8.4.0
> > > > + */
> > > > + VIR_TYPED_PARAM_RESERVED3 = 1 << 1,
> > > >
> > > > /* Older servers lacked the ability to handle string typed
> > > > * parameters. Attempts to set a string parameter with an older
> > > > diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> > > > index 2edef9c4e1..94cb4a6615 100644
> > > > --- a/include/libvirt/libvirt-domain.h
> > > > +++ b/include/libvirt/libvirt-domain.h
> > > > @@ -321,10 +321,10 @@ typedef virDomainControlInfo *virDomainControlInfoPtr;
> > > > * Since: 0.9.2
> > > > */
> > > > typedef enum {
> > > > - VIR_DOMAIN_AFFECT_CURRENT = 0, /* Affect current domain state. (Since: 0.9.2) */
> > > > - VIR_DOMAIN_AFFECT_LIVE = 1 << 0, /* Affect running domain state. (Since: 0.9.2) */
> > > > - VIR_DOMAIN_AFFECT_CONFIG = 1 << 1, /* Affect persistent domain state. (Since: 0.9.2) */
> > > > - /* 1 << 2 is reserved for virTypedParameterFlags */
> > > > + VIR_DOMAIN_AFFECT_CURRENT = 0, /* Affect current domain state. (Since: 0.9.2) */
> > > > + VIR_DOMAIN_AFFECT_LIVE = 1 << 0, /* Affect running domain state. (Since: 0.9.2) */
> > > > + VIR_DOMAIN_AFFECT_CONFIG = 1 << 1, /* Affect persistent domain state. (Since: 0.9.2) */
> > > > + VIR_DOMAIN_AFFECT_RESERVED1 = 1 << 2, /* Reserved for virTypedParameterFlags. Do not use. (Since: 8.4.0) */
> > > > } virDomainModificationImpact;
> > > >
> > > > /**
> > > > --
> > > > 2.35.1
> > > >
> > >
> > > With regards,
> > > Daniel
> >
> > Cheers,
> > Victor
>
> With regards,
> Daniel
Cheers,
Victor
© 2016 - 2026 Red Hat, Inc.