| Message ID | 20190801225251.17864-1-leonardo@linux.ibm.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 9616dda1aaddb2080122aaeab96ad7fc064e36b4 |
| Headers | show |
| Series | [1/1] pseries/hotplug-memory.c: Replace nested ifs by switch-case | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch next (f3365d1a959d5c6527efe3d38276acc9b58e3f3f) |
| snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 52 lines checked |
On 02.08.19 00:52, Leonardo Bras wrote: > I noticed these nested ifs can be easily replaced by switch-cases, > which can improve readability. > > Signed-off-by: Leonardo Bras <leonardo@linux.ibm.com> > --- > .../platforms/pseries/hotplug-memory.c | 26 +++++++++++++------ > 1 file changed, 18 insertions(+), 8 deletions(-) More LOC but seems to be the right thing to do Reviewed-by: David Hildenbrand <david@redhat.com> > > diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c > index 46d0d35b9ca4..8e700390f3d6 100644 > --- a/arch/powerpc/platforms/pseries/hotplug-memory.c > +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c > @@ -880,34 +880,44 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog) > > switch (hp_elog->action) { > case PSERIES_HP_ELOG_ACTION_ADD: > - if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT) { > + switch (hp_elog->id_type) { > + case PSERIES_HP_ELOG_ID_DRC_COUNT: > count = hp_elog->_drc_u.drc_count; > rc = dlpar_memory_add_by_count(count); > - } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX) { > + break; > + case PSERIES_HP_ELOG_ID_DRC_INDEX: > drc_index = hp_elog->_drc_u.drc_index; > rc = dlpar_memory_add_by_index(drc_index); > - } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_IC) { > + break; > + case PSERIES_HP_ELOG_ID_DRC_IC: > count = hp_elog->_drc_u.ic.count; > drc_index = hp_elog->_drc_u.ic.index; > rc = dlpar_memory_add_by_ic(count, drc_index); > - } else { > + break; > + default: > rc = -EINVAL; > + break; > } > > break; > case PSERIES_HP_ELOG_ACTION_REMOVE: > - if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT) { > + switch (hp_elog->id_type) { > + case PSERIES_HP_ELOG_ID_DRC_COUNT: > count = hp_elog->_drc_u.drc_count; > rc = dlpar_memory_remove_by_count(count); > - } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX) { > + break; > + case PSERIES_HP_ELOG_ID_DRC_INDEX: > drc_index = hp_elog->_drc_u.drc_index; > rc = dlpar_memory_remove_by_index(drc_index); > - } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_IC) { > + break; > + case PSERIES_HP_ELOG_ID_DRC_IC: > count = hp_elog->_drc_u.ic.count; > drc_index = hp_elog->_drc_u.ic.index; > rc = dlpar_memory_remove_by_ic(count, drc_index); > - } else { > + break; > + default: > rc = -EINVAL; > + break; > } > > break; >
Leonardo Bras <leonardo@linux.ibm.com> writes: > I noticed these nested ifs can be easily replaced by switch-cases, > which can improve readability. > > Signed-off-by: Leonardo Bras <leonardo@linux.ibm.com> > --- > .../platforms/pseries/hotplug-memory.c | 26 +++++++++++++------ > 1 file changed, 18 insertions(+), 8 deletions(-) Thanks, this looks sensible. Please use "powerpc/" as the prefix on your patches, eg. in this case: "powerpc/pseries/hotplug-memory.c: Replace nested ifs by switch-case" I'll fix it up this time when I apply. cheers > diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c > index 46d0d35b9ca4..8e700390f3d6 100644 > --- a/arch/powerpc/platforms/pseries/hotplug-memory.c > +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c > @@ -880,34 +880,44 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog) > > switch (hp_elog->action) { > case PSERIES_HP_ELOG_ACTION_ADD: > - if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT) { > + switch (hp_elog->id_type) { > + case PSERIES_HP_ELOG_ID_DRC_COUNT: > count = hp_elog->_drc_u.drc_count; > rc = dlpar_memory_add_by_count(count); > - } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX) { > + break; > + case PSERIES_HP_ELOG_ID_DRC_INDEX: > drc_index = hp_elog->_drc_u.drc_index; > rc = dlpar_memory_add_by_index(drc_index); > - } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_IC) { > + break; > + case PSERIES_HP_ELOG_ID_DRC_IC: > count = hp_elog->_drc_u.ic.count; > drc_index = hp_elog->_drc_u.ic.index; > rc = dlpar_memory_add_by_ic(count, drc_index); > - } else { > + break; > + default: > rc = -EINVAL; > + break; > } > > break; > case PSERIES_HP_ELOG_ACTION_REMOVE: > - if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT) { > + switch (hp_elog->id_type) { > + case PSERIES_HP_ELOG_ID_DRC_COUNT: > count = hp_elog->_drc_u.drc_count; > rc = dlpar_memory_remove_by_count(count); > - } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX) { > + break; > + case PSERIES_HP_ELOG_ID_DRC_INDEX: > drc_index = hp_elog->_drc_u.drc_index; > rc = dlpar_memory_remove_by_index(drc_index); > - } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_IC) { > + break; > + case PSERIES_HP_ELOG_ID_DRC_IC: > count = hp_elog->_drc_u.ic.count; > drc_index = hp_elog->_drc_u.ic.index; > rc = dlpar_memory_remove_by_ic(count, drc_index); > - } else { > + break; > + default: > rc = -EINVAL; > + break; > } > > break; > -- > 2.20.1
On Fri, 2019-08-02 at 22:26 +1000, Michael Ellerman wrote: > Leonardo Bras <leonardo@linux.ibm.com> writes: > > I noticed these nested ifs can be easily replaced by switch-cases, > > which can improve readability. > > > > Signed-off-by: Leonardo Bras <leonardo@linux.ibm.com> > > --- > > .../platforms/pseries/hotplug-memory.c | 26 +++++++++++++------ > > 1 file changed, 18 insertions(+), 8 deletions(-) > > Thanks, this looks sensible. > > Please use "powerpc/" as the prefix on your patches, eg. in this case: > > "powerpc/pseries/hotplug-memory.c: Replace nested ifs by switch-case" > Ok, I will make sure to do that next time. Thanks! > I'll fix it up this time when I apply. > > cheers > > > diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c > > index 46d0d35b9ca4..8e700390f3d6 100644 > > --- a/arch/powerpc/platforms/pseries/hotplug-memory.c > > +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c > > @@ -880,34 +880,44 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog) > > > > switch (hp_elog->action) { > > case PSERIES_HP_ELOG_ACTION_ADD: > > - if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT) { > > + switch (hp_elog->id_type) { > > + case PSERIES_HP_ELOG_ID_DRC_COUNT: > > count = hp_elog->_drc_u.drc_count; > > rc = dlpar_memory_add_by_count(count); > > - } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX) { > > + break; > > + case PSERIES_HP_ELOG_ID_DRC_INDEX: > > drc_index = hp_elog->_drc_u.drc_index; > > rc = dlpar_memory_add_by_index(drc_index); > > - } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_IC) { > > + break; > > + case PSERIES_HP_ELOG_ID_DRC_IC: > > count = hp_elog->_drc_u.ic.count; > > drc_index = hp_elog->_drc_u.ic.index; > > rc = dlpar_memory_add_by_ic(count, drc_index); > > - } else { > > + break; > > + default: > > rc = -EINVAL; > > + break; > > } > > > > break; > > case PSERIES_HP_ELOG_ACTION_REMOVE: > > - if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT) { > > + switch (hp_elog->id_type) { > > + case PSERIES_HP_ELOG_ID_DRC_COUNT: > > count = hp_elog->_drc_u.drc_count; > > rc = dlpar_memory_remove_by_count(count); > > - } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX) { > > + break; > > + case PSERIES_HP_ELOG_ID_DRC_INDEX: > > drc_index = hp_elog->_drc_u.drc_index; > > rc = dlpar_memory_remove_by_index(drc_index); > > - } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_IC) { > > + break; > > + case PSERIES_HP_ELOG_ID_DRC_IC: > > count = hp_elog->_drc_u.ic.count; > > drc_index = hp_elog->_drc_u.ic.index; > > rc = dlpar_memory_remove_by_ic(count, drc_index); > > - } else { > > + break; > > + default: > > rc = -EINVAL; > > + break; > > } > > > > break; > > -- > > 2.20.1
On Thu, 2019-08-01 at 22:52:51 UTC, Leonardo Bras wrote: > I noticed these nested ifs can be easily replaced by switch-cases, > which can improve readability. > > Signed-off-by: Leonardo Bras <leonardo@linux.ibm.com> > Reviewed-by: David Hildenbrand <david@redhat.com> Series applied to powerpc next, thanks. https://git.kernel.org/powerpc/c/9616dda1aaddb2080122aaeab96ad7fc064e36b4 cheers
diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c index 46d0d35b9ca4..8e700390f3d6 100644 --- a/arch/powerpc/platforms/pseries/hotplug-memory.c +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c @@ -880,34 +880,44 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog) switch (hp_elog->action) { case PSERIES_HP_ELOG_ACTION_ADD: - if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT) { + switch (hp_elog->id_type) { + case PSERIES_HP_ELOG_ID_DRC_COUNT: count = hp_elog->_drc_u.drc_count; rc = dlpar_memory_add_by_count(count); - } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX) { + break; + case PSERIES_HP_ELOG_ID_DRC_INDEX: drc_index = hp_elog->_drc_u.drc_index; rc = dlpar_memory_add_by_index(drc_index); - } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_IC) { + break; + case PSERIES_HP_ELOG_ID_DRC_IC: count = hp_elog->_drc_u.ic.count; drc_index = hp_elog->_drc_u.ic.index; rc = dlpar_memory_add_by_ic(count, drc_index); - } else { + break; + default: rc = -EINVAL; + break; } break; case PSERIES_HP_ELOG_ACTION_REMOVE: - if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT) { + switch (hp_elog->id_type) { + case PSERIES_HP_ELOG_ID_DRC_COUNT: count = hp_elog->_drc_u.drc_count; rc = dlpar_memory_remove_by_count(count); - } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX) { + break; + case PSERIES_HP_ELOG_ID_DRC_INDEX: drc_index = hp_elog->_drc_u.drc_index; rc = dlpar_memory_remove_by_index(drc_index); - } else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_IC) { + break; + case PSERIES_HP_ELOG_ID_DRC_IC: count = hp_elog->_drc_u.ic.count; drc_index = hp_elog->_drc_u.ic.index; rc = dlpar_memory_remove_by_ic(count, drc_index); - } else { + break; + default: rc = -EINVAL; + break; } break;
I noticed these nested ifs can be easily replaced by switch-cases, which can improve readability. Signed-off-by: Leonardo Bras <leonardo@linux.ibm.com> --- .../platforms/pseries/hotplug-memory.c | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-)