[PATCH v2 0/2] domain_conf: rewriting else after return
diff to v1: * split one commit into two * rewrite the functions in the second one to use the switch (suggested by Daniel, thanks) v1: https://listman.redhat.com/archives/libvir-list/2022-July/232989.html Kristina Hanicova (2): domain_conf: remove else after return / goto domain_conf: rewrite if else functions to switch src/conf/domain_conf.c | 77 +++++++++++++++++++++++++++--------------- 1 file changed, 49 insertions(+), 28 deletions(-) -- 2.35.3
The else branches are redundant because the execution will never reach them if the conditions in the previous 'if' branches are true. I think this looks cleaner and is more readable, because having 'else' branch indicates that no return / break / goto is in the previous branch and the function can reach it. Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 26a241db38..41d23cfe2a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4084,8 +4084,8 @@ virDomainObjGetPersistentDef(virDomainXMLOption *xmlopt, if (domain->newDef) return domain->newDef; - else - return domain->def; + + return domain->def; } @@ -4223,8 +4223,8 @@ virDomainObjGetOneDefState(virDomainObj *vm, if (virDomainObjIsActive(vm) && flags & VIR_DOMAIN_AFFECT_CONFIG) return vm->newDef; - else - return vm->def; + + return vm->def; } @@ -6028,7 +6028,9 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, VIR_XML_PROP_NONZERO, &scsisrc->sgio)) < 0) { return -1; - } else if (rv > 0) { + } + + if (rv > 0) { if (def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { virReportError(VIR_ERR_XML_ERROR, "%s", _("sgio is only supported for scsi host device")); @@ -6040,8 +6042,9 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, VIR_XML_PROP_NONE, &scsisrc->rawio)) < 0) { return -1; - } else if (rv > 0 && - def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { + } + + if (rv > 0 && def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { virReportError(VIR_ERR_XML_ERROR, "%s", _("rawio is only supported for scsi host device")); return -1; @@ -9913,9 +9916,10 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDef *def, ctxt->node = cur; - if ((nsources = virXPathNodeSet("./source", ctxt, &sources)) < 0) { + if ((nsources = virXPathNodeSet("./source", ctxt, &sources)) < 0) goto error; - } else if (nsources > 0) { + + if (nsources > 0) { /* Parse only the first source element since only one is used * for chardev devices, the only exception is UDP type, where * user can specify two source elements. */ @@ -9924,7 +9928,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDef *def, _("only one source element is allowed for " "character device")); goto error; - } else if (nsources > 2) { + } + if (nsources > 2) { virReportError(VIR_ERR_XML_ERROR, "%s", _("only two source elements are allowed for " "character device")); @@ -10004,9 +10009,10 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDef *def, } } - if ((nlogs = virXPathNodeSet("./log", ctxt, &logs)) < 0) { + if ((nlogs = virXPathNodeSet("./log", ctxt, &logs)) < 0) goto error; - } else if (nlogs == 1) { + + if (nlogs == 1) { if (virDomainChrSourceDefParseLog(def, logs[0]) < 0) goto error; } else if (nlogs > 1) { @@ -10016,9 +10022,10 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDef *def, goto error; } - if ((nprotocols = virXPathNodeSet("./protocol", ctxt, &protocols)) < 0) { + if ((nprotocols = virXPathNodeSet("./protocol", ctxt, &protocols)) < 0) goto error; - } else if (nprotocols == 1) { + + if (nprotocols == 1) { if (virDomainChrSourceDefParseProtocol(def, protocols[0]) < 0) goto error; } else if (nprotocols > 1) { -- 2.35.3
On Thu, Jul 21, 2022 at 01:30:44PM +0200, Kristina Hanicova wrote:
The else branches are redundant because the execution will never reach them if the conditions in the previous 'if' branches are true.
I think this looks cleaner and is more readable, because having 'else' branch indicates that no return / break / goto is in the previous branch and the function can reach it.
Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Pattern of using switch instead of a long if else construction is used everywhere, so I used it here as well to make the code more consistent (and remove that else after return). I also included all the values from the enum. Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 42 ++++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 41d23cfe2a..a4b3a49086 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8055,19 +8055,26 @@ static int virDomainControllerModelTypeFromString(const virDomainControllerDef *def, const char *model) { - if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) + switch ((virDomainControllerType)def->type) { + case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: return virDomainControllerModelSCSITypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) + case VIR_DOMAIN_CONTROLLER_TYPE_USB: return virDomainControllerModelUSBTypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) + case VIR_DOMAIN_CONTROLLER_TYPE_PCI: return virDomainControllerModelPCITypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) + case VIR_DOMAIN_CONTROLLER_TYPE_IDE: return virDomainControllerModelIDETypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) + case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: return virDomainControllerModelVirtioSerialTypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA) + case VIR_DOMAIN_CONTROLLER_TYPE_ISA: return virDomainControllerModelISATypeFromString(model); - + case VIR_DOMAIN_CONTROLLER_TYPE_FDC: + case VIR_DOMAIN_CONTROLLER_TYPE_SATA: + case VIR_DOMAIN_CONTROLLER_TYPE_CCID: + case VIR_DOMAIN_CONTROLLER_TYPE_XENBUS: + case VIR_DOMAIN_CONTROLLER_TYPE_LAST: + return -1; + } return -1; } @@ -8076,19 +8083,26 @@ static const char * virDomainControllerModelTypeToString(virDomainControllerDef *def, int model) { - if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) + switch ((virDomainControllerType)def->type) { + case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: return virDomainControllerModelSCSITypeToString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) + case VIR_DOMAIN_CONTROLLER_TYPE_USB: return virDomainControllerModelUSBTypeToString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) + case VIR_DOMAIN_CONTROLLER_TYPE_PCI: return virDomainControllerModelPCITypeToString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) + case VIR_DOMAIN_CONTROLLER_TYPE_IDE: return virDomainControllerModelIDETypeToString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) + case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: return virDomainControllerModelVirtioSerialTypeToString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA) + case VIR_DOMAIN_CONTROLLER_TYPE_ISA: return virDomainControllerModelISATypeToString(model); - + case VIR_DOMAIN_CONTROLLER_TYPE_FDC: + case VIR_DOMAIN_CONTROLLER_TYPE_SATA: + case VIR_DOMAIN_CONTROLLER_TYPE_CCID: + case VIR_DOMAIN_CONTROLLER_TYPE_XENBUS: + case VIR_DOMAIN_CONTROLLER_TYPE_LAST: + return NULL; + } return NULL; } -- 2.35.3
On Thu, Jul 21, 2022 at 01:30:45PM +0200, Kristina Hanicova wrote:
Pattern of using switch instead of a long if else construction is used everywhere, so I used it here as well to make the code more consistent (and remove that else after return). I also included all the values from the enum.
The commit messages are usually non-subjective, as in: "... use it here..." and "Also include ...", but it's not a rule.
Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
participants (2)
-
Kristina Hanicova -
Martin Kletzander