[libvirt] Dead code vs. XXX comment: remove or not?
clang reported that this assignment to type is a dead store, since type is never used after this point. This is xm_internal.c, line 1074: /* XXX Forcing to pretend its a bridge */ if (type == -1) { type = 1; } Normally I'd just remove the whole if block, but the XXX comment makes me think someone has plans for this code. I'll wait for a second opinion.
On Thu, Sep 03, 2009 at 06:28:13PM +0200, Jim Meyering wrote:
clang reported that this assignment to type is a dead store, since type is never used after this point.
This is xm_internal.c, line 1074:
/* XXX Forcing to pretend its a bridge */ if (type == -1) { type = 1; }
Normally I'd just remove the whole if block, but the XXX comment makes me think someone has plans for this code.
I'll wait for a second opinion.
Seems to me we used to expect type would be used to keep the trace of the type of the vif, but we never use that variable, only for that test. I would just kill it from that block list = virConfGetValue(conf, "vif"); if (list && list->type == VIR_CONF_LIST) { list = list->list; while (list) { int type = -1; since we never use the value anywhere ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Daniel Veillard wrote:
On Thu, Sep 03, 2009 at 06:28:13PM +0200, Jim Meyering wrote:
clang reported that this assignment to type is a dead store, since type is never used after this point.
This is xm_internal.c, line 1074:
/* XXX Forcing to pretend its a bridge */ if (type == -1) { type = 1; }
Normally I'd just remove the whole if block, but the XXX comment makes me think someone has plans for this code.
I'll wait for a second opinion.
Seems to me we used to expect type would be used to keep the trace of the type of the vif, but we never use that variable, only for that test. I would just kill it from that block
list = virConfGetValue(conf, "vif"); if (list && list->type == VIR_CONF_LIST) { list = list->list; while (list) { int type = -1;
since we never use the value anywhere !
Good. Thanks for the reply. Here's the patch.
From 7280a306ba98ea6fa3d322089c3193b114ca4da1 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Fri, 4 Sep 2009 16:49:37 +0200 Subject: [PATCH] xm_internal.c: remove dead stores of local, "type"
* src/xm_internal.c (xenXMDomainConfigParse): Remove declaration and useless containing if-block, too. --- src/xm_internal.c | 9 +-------- 1 files changed, 1 insertions(+), 8 deletions(-) diff --git a/src/xm_internal.c b/src/xm_internal.c index 2595a5e..e675672 100644 --- a/src/xm_internal.c +++ b/src/xm_internal.c @@ -1,7 +1,7 @@ /* * xm_internal.h: helper routines for dealing with inactive domains * - * Copyright (C) 2006-2007 Red Hat + * Copyright (C) 2006-2007, 2009 Red Hat * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -995,7 +995,6 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { if (list && list->type == VIR_CONF_LIST) { list = list->list; while (list) { - int type = -1; char script[PATH_MAX]; char model[10]; char ip[16]; @@ -1031,7 +1030,6 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { mac[len] = '\0'; } else if (STRPREFIX(key, "bridge=")) { int len = nextkey ? (nextkey - data) : sizeof(bridge)-1; - type = 1; if (len > (sizeof(bridge)-1)) len = sizeof(bridge)-1; strncpy(bridge, data, len); @@ -1069,11 +1067,6 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { key = nextkey; } - /* XXX Forcing to pretend its a bridge */ - if (type == -1) { - type = 1; - } - if (VIR_ALLOC(net) < 0) goto cleanup; -- 1.6.4.2.409.g85dc3
On Fri, Sep 04, 2009 at 04:52:07PM +0200, Jim Meyering wrote:
Daniel Veillard wrote:
On Thu, Sep 03, 2009 at 06:28:13PM +0200, Jim Meyering wrote:
clang reported that this assignment to type is a dead store, since type is never used after this point.
This is xm_internal.c, line 1074:
/* XXX Forcing to pretend its a bridge */ if (type == -1) { type = 1; }
Normally I'd just remove the whole if block, but the XXX comment makes me think someone has plans for this code.
I'll wait for a second opinion.
Seems to me we used to expect type would be used to keep the trace of the type of the vif, but we never use that variable, only for that test. I would just kill it from that block
list = virConfGetValue(conf, "vif"); if (list && list->type == VIR_CONF_LIST) { list = list->list; while (list) { int type = -1;
since we never use the value anywhere !
Good. Thanks for the reply. Here's the patch.
From 7280a306ba98ea6fa3d322089c3193b114ca4da1 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Fri, 4 Sep 2009 16:49:37 +0200 Subject: [PATCH] xm_internal.c: remove dead stores of local, "type"
* src/xm_internal.c (xenXMDomainConfigParse): Remove declaration and useless containing if-block, too. --- src/xm_internal.c | 9 +-------- 1 files changed, 1 insertions(+), 8 deletions(-)
Yup, ACK :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (2)
-
Daniel Veillard -
Jim Meyering