[libvirt PATCH 0/2] API documentation fixes
Andrea Bolognani (2): lib: Drop "type hint" comments include: Explicitly reserve values for overlapping flag types include/libvirt/libvirt-common.h.in | 19 +++++++++++++++++-- include/libvirt/libvirt-domain.h | 8 ++++---- src/libvirt-network.c | 4 ++-- 3 files changed, 23 insertions(+), 8 deletions(-) -- 2.35.1
Their presence confuses apibuild.py, which considers them to be part of the previous comment block and so appends them verbatim to the documentation for virNetworkUpdate(). Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/libvirt-network.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libvirt-network.c b/src/libvirt-network.c index 50c4108641..dc50f33eba 100644 --- a/src/libvirt-network.c +++ b/src/libvirt-network.c @@ -637,8 +637,8 @@ virNetworkUndefine(virNetworkPtr network) */ int virNetworkUpdate(virNetworkPtr network, - unsigned int command, /* virNetworkUpdateCommand */ - unsigned int section, /* virNetworkUpdateSection */ + unsigned int command, + unsigned int section, int parentIndex, const char *xml, unsigned int flags) -- 2.35.1
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.1
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 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 :|
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
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 :|
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Victor Toso