[libvirt] [PATCH 0/4] Couple of trivial c89 style fixes
*** BLURB HERE *** BLURB Michal Privoznik (4): examples: Properly include getopt.h vmwarevertest: drop VIR_FROM_THIS definition lib: Fix c99 style comments cpu: Avoid c99 style of assembler examples/admin/logging.c | 14 +++++++------- examples/openauth/openauth.c | 2 +- src/cpu/cpu_ppc64.c | 4 ++-- src/cpu/cpu_x86.c | 44 +++++++++++++++++++++---------------------- src/lxc/lxc_controller.c | 2 +- src/remote/remote_driver.c | 2 +- src/rpc/virnetservermdns.c | 2 +- src/security/security_stack.c | 8 ++++---- src/uml/uml_conf.c | 2 +- src/util/virhostcpu.c | 4 ++-- src/util/virhostmem.c | 4 ++-- src/vbox/vbox_snapshot_conf.c | 2 +- tests/eventtest.c | 2 +- tests/virhostcpumock.c | 2 +- tests/vmwarevertest.c | 2 -- 15 files changed, 47 insertions(+), 49 deletions(-) -- 2.10.2
In the admin/logging.c example, the getopt() function is used but without proper #include. It relies on unistd.h to subsequently include getopt.h. This is not necessarily always the case. Also, opterr is not needed and actually not used anywhere else in our code. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- examples/admin/logging.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/examples/admin/logging.c b/examples/admin/logging.c index 648d7a6..de258c2 100644 --- a/examples/admin/logging.c +++ b/examples/admin/logging.c @@ -1,11 +1,12 @@ -#include<stdio.h> -#include<stdlib.h> -#include<stdbool.h> +#include <stdio.h> +#include <stdlib.h> +#include <stdbool.h> #include "config.h" -#include<unistd.h> -#include<libvirt/libvirt-admin.h> -#include<libvirt/virterror.h> +#include <unistd.h> +#include <getopt.h> +#include <libvirt/libvirt-admin.h> +#include <libvirt/virterror.h> static void printHelp(const char *argv0) { @@ -31,7 +32,6 @@ int main(int argc, char **argv) const char *set_filters = NULL; ret = c = -1; - opterr = 0; while ((c = getopt(argc, argv, ":hpo:f:")) > 0) { switch (c) { -- 2.10.2
On Thu, Apr 27, 2017 at 09:01:57 +0200, Michal Privoznik wrote:
In the admin/logging.c example, the getopt() function is used but without proper #include. It relies on unistd.h to subsequently include getopt.h. This is not necessarily always the case. Also, opterr is not needed and actually not used anywhere else in our code.
man 3 opterr: * If the caller has set the global variable opterr to zero, then * getopt() does not print an error message. The caller can determine that there was an error by testing whether the function return value is '?'. (By default, opterr has a nonzero value.)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- examples/admin/logging.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/examples/admin/logging.c b/examples/admin/logging.c index 648d7a6..de258c2 100644 --- a/examples/admin/logging.c +++ b/examples/admin/logging.c @@ -1,11 +1,12 @@ -#include<stdio.h> -#include<stdlib.h> -#include<stdbool.h> +#include <stdio.h> +#include <stdlib.h> +#include <stdbool.h>
#include "config.h" -#include<unistd.h> -#include<libvirt/libvirt-admin.h> -#include<libvirt/virterror.h> +#include <unistd.h> +#include <getopt.h> +#include <libvirt/libvirt-admin.h> +#include <libvirt/virterror.h>
static void printHelp(const char *argv0) { @@ -31,7 +32,6 @@ int main(int argc, char **argv) const char *set_filters = NULL;
ret = c = -1; - opterr = 0;
NACK to this.
Firstly, this definition is in a c99 comment, secondly it is not needed as VIR_FROM_THIS is defined from vmware/vmware_conf.h. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/vmwarevertest.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/vmwarevertest.c b/tests/vmwarevertest.c index 3dbe08b..e0e36fe 100644 --- a/tests/vmwarevertest.c +++ b/tests/vmwarevertest.c @@ -27,8 +27,6 @@ # include "vmware/vmware_conf.h" -//# define VIR_FROM_THIS VIR_FROM_NONE - struct testInfo { const char *vmware_type; const char *name; -- 2.10.2
On Thu, Apr 27, 2017 at 09:01:58 +0200, Michal Privoznik wrote:
Firstly, this definition is in a c99 comment, secondly it is not needed as VIR_FROM_THIS is defined from vmware/vmware_conf.h.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/vmwarevertest.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/tests/vmwarevertest.c b/tests/vmwarevertest.c index 3dbe08b..e0e36fe 100644 --- a/tests/vmwarevertest.c +++ b/tests/vmwarevertest.c @@ -27,8 +27,6 @@
# include "vmware/vmware_conf.h"
-//# define VIR_FROM_THIS VIR_FROM_NONE -
This test uses the virAsprintf macro which uses the VIR_FROM_THIS macro. I think the test file should set it explicitly rather than depend on the one defined by including the header.
On Thu, Apr 27, 2017 at 09:36:19AM +0200, Peter Krempa wrote:
On Thu, Apr 27, 2017 at 09:01:58 +0200, Michal Privoznik wrote:
Firstly, this definition is in a c99 comment, secondly it is not needed as VIR_FROM_THIS is defined from vmware/vmware_conf.h.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/vmwarevertest.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/tests/vmwarevertest.c b/tests/vmwarevertest.c index 3dbe08b..e0e36fe 100644 --- a/tests/vmwarevertest.c +++ b/tests/vmwarevertest.c @@ -27,8 +27,6 @@
# include "vmware/vmware_conf.h"
-//# define VIR_FROM_THIS VIR_FROM_NONE -
This test uses the virAsprintf macro which uses the VIR_FROM_THIS macro. I think the test file should set it explicitly rather than depend on the one defined by including the header.
Almost all our code puts the VIR_FROM_THIS define in the .c file, so IMHO we should fix vmware_conf.h to match 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 :|
We prefer c89 style of comments. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- examples/openauth/openauth.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/remote/remote_driver.c | 2 +- src/rpc/virnetservermdns.c | 2 +- src/security/security_stack.c | 8 ++++---- src/uml/uml_conf.c | 2 +- src/util/virhostcpu.c | 4 ++-- src/util/virhostmem.c | 4 ++-- src/vbox/vbox_snapshot_conf.c | 2 +- tests/eventtest.c | 2 +- tests/virhostcpumock.c | 2 +- 11 files changed, 16 insertions(+), 16 deletions(-) diff --git a/examples/openauth/openauth.c b/examples/openauth/openauth.c index 7a8254b..eef46d5 100644 --- a/examples/openauth/openauth.c +++ b/examples/openauth/openauth.c @@ -216,7 +216,7 @@ static virConnectAuth auth = { credTypes, sizeof(credTypes) / sizeof(int), authCallback, - NULL, // cbdata will be initialized in main + NULL, /* cbdata will be initialized in main */ }; diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 88ba8aa..1b9d215 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -1800,7 +1800,7 @@ virLXCControllerSetupHostdevCaps(virDomainDefPtr vmDef, securityDriver); case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_NET: - return 0; // case is handled in virLXCControllerMoveInterfaces + return 0; /* case is handled in virLXCControllerMoveInterfaces */ default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 1242bd6..77250ea 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1145,7 +1145,7 @@ doRemoteOpen(virConnectPtr conn, if (!(priv->closeCallback = virNewConnectCloseCallbackData())) goto failed; - // ref on behalf of netclient + /* ref on behalf of netclient */ virObjectRef(priv->closeCallback); virNetClientSetCloseCallback(priv->client, remoteClientCloseFunc, diff --git a/src/rpc/virnetservermdns.c b/src/rpc/virnetservermdns.c index 7f12a29..8d7df24 100644 --- a/src/rpc/virnetservermdns.c +++ b/src/rpc/virnetservermdns.c @@ -128,7 +128,7 @@ static void virNetServerMDNSGroupCallback(AvahiEntryGroup *g ATTRIBUTE_UNUSED, avahi_strerror(avahi_client_errno(group->mdns->client))); /* Some kind of failure happened while we were registering our services */ - //avahi_simple_poll_quit(simple_poll); + /* avahi_simple_poll_quit(simple_poll); */ break; case AVAHI_ENTRY_GROUP_UNCOMMITED: diff --git a/src/security/security_stack.c b/src/security/security_stack.c index b02ee18..9a1a7b3 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -210,7 +210,7 @@ virSecurityStackGenLabel(virSecurityManagerPtr mgr, if (virSecurityManagerGenLabel(virSecurityStackGetPrimary(mgr), vm) < 0) rc = -1; -// TODO +/* TODO */ #if 0 /* We don't allow secondary drivers to generate labels. * This may have to change in the future, but requires @@ -235,7 +235,7 @@ virSecurityStackReleaseLabel(virSecurityManagerPtr mgr, if (virSecurityManagerReleaseLabel(virSecurityStackGetPrimary(mgr), vm) < 0) rc = -1; -// TODO +/* TODO */ #if 0 /* XXX See note in GenLabel */ if (virSecurityManagerReleaseLabel(priv->secondary, vm) < 0) @@ -255,7 +255,7 @@ virSecurityStackReserveLabel(virSecurityManagerPtr mgr, if (virSecurityManagerReserveLabel(virSecurityStackGetPrimary(mgr), vm, pid) < 0) rc = -1; -// TODO +/* TODO */ #if 0 /* XXX See note in GenLabel */ if (virSecurityManagerReserveLabel(priv->secondary, vm, pid) < 0) @@ -460,7 +460,7 @@ virSecurityStackGetProcessLabel(virSecurityManagerPtr mgr, { int rc = 0; -// TODO +/* TODO */ #if 0 if (virSecurityManagerGetProcessLabel(priv->secondary, vm, pid, seclabel) < 0) rc = -1; diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index bdef783..d223af0 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -399,7 +399,7 @@ virCommandPtr umlBuildCommandLine(virConnectPtr conn, virCommandAddEnvPassCommon(cmd); - //virCommandAddArgPair(cmd, "con0", "fd:0,fd:1"); + /* virCommandAddArgPair(cmd, "con0", "fd:0,fd:1"); */ virCommandAddArgFormat(cmd, "mem=%lluK", vm->def->mem.cur_balloon); virCommandAddArgPair(cmd, "umid", vm->def->name); virCommandAddArgPair(cmd, "uml_dir", driver->monitorDir); diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index a660e3f..8397307 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -795,8 +795,8 @@ virHostCPUGetStatsLinux(FILE *procstat, if (STRPREFIX(buf, cpu_header)) { /* aka logical CPU time */ if (sscanf(buf, - "%*s %llu %llu %llu %llu %llu" // user ~ iowait - "%llu %llu %llu %llu %llu", // irq ~ guest_nice + "%*s %llu %llu %llu %llu %llu" /* user ~ iowait */ + "%llu %llu %llu %llu %llu", /* irq ~ guest_nice */ &usr, &ni, &sys, &idle, &iowait, &irq, &softirq, &steal, &guest, &guest_nice) < 4) { continue; diff --git a/src/util/virhostmem.c b/src/util/virhostmem.c index 2ac90a0..a9ba278 100644 --- a/src/util/virhostmem.c +++ b/src/util/virhostmem.c @@ -158,8 +158,8 @@ virHostMemGetStatsLinux(FILE *meminfo, char meminfo_hdr[VIR_NODE_MEMORY_STATS_FIELD_LENGTH]; unsigned long val; struct field_conv { - const char *meminfo_hdr; // meminfo header - const char *field; // MemoryStats field name + const char *meminfo_hdr; /* meminfo header */ + const char *field; /* MemoryStats field name */ } field_conv[] = { {"MemTotal:", VIR_NODE_MEMORY_STATS_TOTAL}, {"MemFree:", VIR_NODE_MEMORY_STATS_FREE}, diff --git a/src/vbox/vbox_snapshot_conf.c b/src/vbox/vbox_snapshot_conf.c index 8bf7ef3..c3b2855 100644 --- a/src/vbox/vbox_snapshot_conf.c +++ b/src/vbox/vbox_snapshot_conf.c @@ -958,7 +958,7 @@ virVBoxSnapshotConfRemoveHardDisk(virVBoxSnapshotConfMediaRegistryPtr mediaRegis goto cleanup; } if (hardDisk->parent == NULL) { - //it means that the hard disk is in 'root' + /* it means that the hard disk is in 'root' */ for (i = 0; i < mediaRegistry->ndisks; i++) { if (hardDisk == mediaRegistry->disks[i]) break; diff --git a/tests/eventtest.c b/tests/eventtest.c index ea47fb0..0c1ec71 100644 --- a/tests/eventtest.c +++ b/tests/eventtest.c @@ -523,7 +523,7 @@ mymain(void) if (finishJob("Write duplicate", 1, -1) != EXIT_SUCCESS) return EXIT_FAILURE; - //pthread_kill(eventThread, SIGTERM); + /* pthread_kill(eventThread, SIGTERM); */ return EXIT_SUCCESS; } diff --git a/tests/virhostcpumock.c b/tests/virhostcpumock.c index 00d10f1..48147b2 100644 --- a/tests/virhostcpumock.c +++ b/tests/virhostcpumock.c @@ -26,7 +26,7 @@ virHostCPUGetThreadsPerSubcore(virArch arch) { int threads_per_subcore = 0; - // Emulate SMT=8 on POWER hardware + /* Emulate SMT=8 on POWER hardware */ if (ARCH_IS_PPC64(arch)) threads_per_subcore = 8; -- 2.10.2
On Thu, Apr 27, 2017 at 09:01:59 +0200, Michal Privoznik wrote:
We prefer c89 style of comments.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- examples/openauth/openauth.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/remote/remote_driver.c | 2 +- src/rpc/virnetservermdns.c | 2 +- src/security/security_stack.c | 8 ++++---- src/uml/uml_conf.c | 2 +- src/util/virhostcpu.c | 4 ++-- src/util/virhostmem.c | 4 ++-- src/vbox/vbox_snapshot_conf.c | 2 +- tests/eventtest.c | 2 +- tests/virhostcpumock.c | 2 +- 11 files changed, 16 insertions(+), 16 deletions(-)
ACK
On Thu, Apr 27, 2017 at 09:01:59AM +0200, Michal Privoznik wrote:
We prefer c89 style of comments.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- examples/openauth/openauth.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/remote/remote_driver.c | 2 +- src/rpc/virnetservermdns.c | 2 +- src/security/security_stack.c | 8 ++++---- src/uml/uml_conf.c | 2 +- src/util/virhostcpu.c | 4 ++-- src/util/virhostmem.c | 4 ++-- src/vbox/vbox_snapshot_conf.c | 2 +- tests/eventtest.c | 2 +- tests/virhostcpumock.c | 2 +- 11 files changed, 16 insertions(+), 16 deletions(-)
How about a syntax-check rule to enforce this ? 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 04/27/2017 10:19 AM, Daniel P. Berrange wrote:
On Thu, Apr 27, 2017 at 09:01:59AM +0200, Michal Privoznik wrote:
We prefer c89 style of comments.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- examples/openauth/openauth.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/remote/remote_driver.c | 2 +- src/rpc/virnetservermdns.c | 2 +- src/security/security_stack.c | 8 ++++---- src/uml/uml_conf.c | 2 +- src/util/virhostcpu.c | 4 ++-- src/util/virhostmem.c | 4 ++-- src/vbox/vbox_snapshot_conf.c | 2 +- tests/eventtest.c | 2 +- tests/virhostcpumock.c | 2 +- 11 files changed, 16 insertions(+), 16 deletions(-)
How about a syntax-check rule to enforce this ?
It's going to be not trivial. Consider: /* My awesome comment, that contains * http://an.url.com */ It's not easy to know that line 2 of the comment is already in comment. We can't just forbid all '//' strings. Michal
On Thu, Apr 27, 2017 at 11:09:29AM +0200, Michal Privoznik wrote:
On 04/27/2017 10:19 AM, Daniel P. Berrange wrote:
On Thu, Apr 27, 2017 at 09:01:59AM +0200, Michal Privoznik wrote:
We prefer c89 style of comments.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- examples/openauth/openauth.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/remote/remote_driver.c | 2 +- src/rpc/virnetservermdns.c | 2 +- src/security/security_stack.c | 8 ++++---- src/uml/uml_conf.c | 2 +- src/util/virhostcpu.c | 4 ++-- src/util/virhostmem.c | 4 ++-- src/vbox/vbox_snapshot_conf.c | 2 +- tests/eventtest.c | 2 +- tests/virhostcpumock.c | 2 +- 11 files changed, 16 insertions(+), 16 deletions(-)
How about a syntax-check rule to enforce this ?
It's going to be not trivial. Consider:
/* My awesome comment, that contains * http://an.url.com */
It's not easy to know that line 2 of the comment is already in comment. We can't just forbid all '//' strings.
Our build-aux/bracket-spacing.pl script knows how to strip out /* ... */ It also strips // out, so we could just adapt it to whine when it sees // comments, rather than try to do it using the cfg.mk rules. 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 :|
In c89 - the standard we claim to support - there is no asm() rather than __asm__(). Switch from the former to the latter. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/cpu/cpu_ppc64.c | 4 ++-- src/cpu/cpu_x86.c | 44 ++++++++++++++++++++++---------------------- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index f64592b..5d65929 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -732,8 +732,8 @@ virCPUppc64GetHost(virCPUDefPtr cpu, data->len = 1; #if defined(__powerpc__) || defined(__powerpc64__) - asm("mfpvr %0" - : "=r" (data->pvr[0].value)); + __asm__("mfpvr %0" + : "=r" (data->pvr[0].value)); #endif data->pvr[0].mask = 0xfffffffful; diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 53359ff..c971aa3 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -2085,32 +2085,32 @@ static inline void cpuidCall(virCPUx86CPUID *cpuid) { # if __x86_64__ - asm("xor %%ebx, %%ebx;" /* clear the other registers as some cpuid */ - "xor %%edx, %%edx;" /* functions may use them as additional arguments */ - "cpuid;" - : "=a" (cpuid->eax), - "=b" (cpuid->ebx), - "=c" (cpuid->ecx), - "=d" (cpuid->edx) - : "a" (cpuid->eax_in), - "c" (cpuid->ecx_in)); + __asm__("xor %%ebx, %%ebx;" /* clear the other registers as some cpuid */ + "xor %%edx, %%edx;" /* functions may use them as additional arguments */ + "cpuid;" + : "=a" (cpuid->eax), + "=b" (cpuid->ebx), + "=c" (cpuid->ecx), + "=d" (cpuid->edx) + : "a" (cpuid->eax_in), + "c" (cpuid->ecx_in)); # else /* we need to avoid direct use of ebx for CPUID output as it is used * for global offset table on i386 with -fPIC */ - asm("push %%ebx;" - "xor %%ebx, %%ebx;" /* clear the other registers as some cpuid */ - "xor %%edx, %%edx;" /* functions may use them as additional arguments */ - "cpuid;" - "mov %%ebx, %1;" - "pop %%ebx;" - : "=a" (cpuid->eax), - "=r" (cpuid->ebx), - "=c" (cpuid->ecx), - "=d" (cpuid->edx) - : "a" (cpuid->eax_in), - "c" (cpuid->ecx_in) - : "cc"); + __asm__("push %%ebx;" + "xor %%ebx, %%ebx;" /* clear the other registers as some cpuid */ + "xor %%edx, %%edx;" /* functions may use them as additional arguments */ + "cpuid;" + "mov %%ebx, %1;" + "pop %%ebx;" + : "=a" (cpuid->eax), + "=r" (cpuid->ebx), + "=c" (cpuid->ecx), + "=d" (cpuid->edx) + : "a" (cpuid->eax_in), + "c" (cpuid->ecx_in) + : "cc"); # endif } -- 2.10.2
On Thu, Apr 27, 2017 at 09:02:00 +0200, Michal Privoznik wrote:
In c89 - the standard we claim to support - there is no asm()
I'm not quite sure about the truth of this statement. Especially after commits like: 0f6c46c3d virsh.c: Switch to C99 initialization of vshCmdOptDef 879d409e9 Convert all driver struct intializers to C99 style and quite a lot more ...
rather than __asm__(). Switch from the former to the latter.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/cpu/cpu_ppc64.c | 4 ++-- src/cpu/cpu_x86.c | 44 ++++++++++++++++++++++---------------------- 2 files changed, 24 insertions(+), 24 deletions(-)
On Thu, Apr 27, 2017 at 09:24:54AM +0200, Peter Krempa wrote:
On Thu, Apr 27, 2017 at 09:02:00 +0200, Michal Privoznik wrote:
In c89 - the standard we claim to support - there is no asm()
I'm not quite sure about the truth of this statement. Especially after commits like:
0f6c46c3d virsh.c: Switch to C99 initialization of vshCmdOptDef 879d409e9 Convert all driver struct intializers to C99 style
and quite a lot more ...
Yep the C99 struct member initializers are a very valuable feature. I'd say, we aim for c89, plus selected c99 features. We explicitly try to avoid some other c99 features though, such variables declared in the middle of functions. 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, Apr 27, 2017 at 09:22:04AM +0100, Daniel P. Berrange wrote:
On Thu, Apr 27, 2017 at 09:24:54AM +0200, Peter Krempa wrote:
On Thu, Apr 27, 2017 at 09:02:00 +0200, Michal Privoznik wrote:
In c89 - the standard we claim to support - there is no asm()
I'm not quite sure about the truth of this statement. Especially after commits like:
0f6c46c3d virsh.c: Switch to C99 initialization of vshCmdOptDef 879d409e9 Convert all driver struct intializers to C99 style
and quite a lot more ...
Yep the C99 struct member initializers are a very valuable feature. I'd say, we aim for c89, plus selected c99 features.
We explicitly try to avoid some other c99 features though, such variables declared in the middle of functions.
Looking at the gcc/clang docs... By default GCC applies a C standard called gnu89/gnu90, which is basically c89/c90, but with a tonne of GCC extensions, many of which are present in the newer c99. Actually using c89 is impractical since it lacks even basic things like "inline" and var-args for macros. CLang aims to be compatible with GCC extensions, but in fact defaults to the even newer gnu11 standard, which is C11 + GCC extensions It looks like the only GCC extensions we rely on from gnu89, are all present in c99 - at least build succeeds with both std=gnu89 and std=c99 So, it is probably accurate to say we're compliant with c99, and if a compiler doesn't support c99, it must support the GNU extensions to c89. In practice we only care about GCC & CLang. So I guess the question from thsi patch series is mostly, which c99 features do we wish to explicitly avoid using. Variable declarations in the middle of blocks is one we avoid, just because it harms readability and understanding of code when goto is just to jump over a variable declaration. I see no problem using c99 asm statements, unless we're using a mix of different asm syntaxes across the code base, in which case we should standardize on one. I tend to think it is worth avoiding C++ comments, just for sake of having a consistent style across the code base. 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)
-
Daniel P. Berrange -
Michal Privoznik -
Peter Krempa