Re: [PATCH] domain_conf: remove else after return / goto

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On Wed, Jul 20, 2022 at 3:34 PM Daniel P. Berrangé <berrange@xxxxxxxxxx> wrote:

What's the reasoning for making this change ?

I stumbled upon this and decided to rewrite code in src/conf/ that could be easily improved.
 

On Wed, Jul 20, 2022 at 02:59:58PM +0200, Kristina Hanicova wrote:
> Signed-off-by: Kristina Hanicova <khanicov@xxxxxxxxxx>
> ---
>  src/conf/domain_conf.c | 55 ++++++++++++++++++++++++------------------
>  1 file changed, 31 insertions(+), 24 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 44a01ab628..bc4b74c1c8 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;
>  }


I'm not really convinced these two changes are better.

Well, the else after return is redundant because it will never reach the 'else' branch if the condition is true.

I think this looks cleaner and is more readable, because having 'else' branch indicates to me that no return / break / goto is in the previous branch and the funcion can reach it.
 



> @@ -6029,7 +6029,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"));
> @@ -6041,8 +6043,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;


I don't mind eliminating the else, when the first 'if' is just an
error return/goto case.

> @@ -8056,15 +8059,15 @@ virDomainControllerModelTypeFromString(const virDomainControllerDef *def,
>  {
>      if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI)
>          return virDomainControllerModelSCSITypeFromString(model);
> -    else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB)
> +    if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB)
>          return virDomainControllerModelUSBTypeFromString(model);
> -    else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
> +    if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
>          return virDomainControllerModelPCITypeFromString(model);
> -    else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
> +    if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
>          return virDomainControllerModelIDETypeFromString(model);
> -    else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
> +    if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
>          return virDomainControllerModelVirtioSerialTypeFromString(model);
> -    else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA)
> +    if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA)
>          return virDomainControllerModelISATypeFromString(model);

This giant if/else should be a switch instead

Thanks, way better idea.
 


>      return -1;
> @@ -8077,15 +8080,15 @@ virDomainControllerModelTypeToString(virDomainControllerDef *def,
>  {
>      if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI)
>          return virDomainControllerModelSCSITypeToString(model);
> -    else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB)
> +    if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB)
>          return virDomainControllerModelUSBTypeToString(model);
> -    else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
> +    if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
>          return virDomainControllerModelPCITypeToString(model);
> -    else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
> +    if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
>          return virDomainControllerModelIDETypeToString(model);
> -    else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
> +    if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
>          return virDomainControllerModelVirtioSerialTypeToString(model);
> -    else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA)
> +    if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA)
>          return virDomainControllerModelISATypeToString(model);

Likewise a switch.


>      return NULL;
> @@ -9915,9 +9918,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. */
> @@ -9926,7 +9930,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"));
> @@ -10006,9 +10011,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) {
> @@ -10018,9 +10024,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) {


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 :|



Regards,
Kristina

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux