[PATCH v1 1/1] libnvdimm/labels: Get rid of redundant 'else'

[PATCH v1 1/1] libnvdimm/labels: Get rid of redundant 'else'

[Andy Shevchenko]

In the snippets like the following

	if (...)
		return / goto / break / continue ...;
	else
		...

the 'else' is redundant. Get rid of it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/nvdimm/label.c | 60 ++++++++++++++++++++----------------------
 1 file changed, 29 insertions(+), 31 deletions(-)

diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c
index 04f4a049599a..b129f3a55a70 100644
--- a/drivers/nvdimm/label.c
+++ b/drivers/nvdimm/label.c
@@ -734,13 +734,13 @@ static enum nvdimm_claim_class guid_to_nvdimm_cclass(guid_t *guid)
 {
 	if (guid_equal(guid, &nvdimm_btt_guid))
 		return NVDIMM_CCLASS_BTT;
-	else if (guid_equal(guid, &nvdimm_btt2_guid))
+	if (guid_equal(guid, &nvdimm_btt2_guid))
 		return NVDIMM_CCLASS_BTT2;
-	else if (guid_equal(guid, &nvdimm_pfn_guid))
+	if (guid_equal(guid, &nvdimm_pfn_guid))
 		return NVDIMM_CCLASS_PFN;
-	else if (guid_equal(guid, &nvdimm_dax_guid))
+	if (guid_equal(guid, &nvdimm_dax_guid))
 		return NVDIMM_CCLASS_DAX;
-	else if (guid_equal(guid, &guid_null))
+	if (guid_equal(guid, &guid_null))
 		return NVDIMM_CCLASS_NONE;
 
 	return NVDIMM_CCLASS_UNKNOWN;
@@ -751,13 +751,13 @@ static enum nvdimm_claim_class uuid_to_nvdimm_cclass(uuid_t *uuid)
 {
 	if (uuid_equal(uuid, &nvdimm_btt_uuid))
 		return NVDIMM_CCLASS_BTT;
-	else if (uuid_equal(uuid, &nvdimm_btt2_uuid))
+	if (uuid_equal(uuid, &nvdimm_btt2_uuid))
 		return NVDIMM_CCLASS_BTT2;
-	else if (uuid_equal(uuid, &nvdimm_pfn_uuid))
+	if (uuid_equal(uuid, &nvdimm_pfn_uuid))
 		return NVDIMM_CCLASS_PFN;
-	else if (uuid_equal(uuid, &nvdimm_dax_uuid))
+	if (uuid_equal(uuid, &nvdimm_dax_uuid))
 		return NVDIMM_CCLASS_DAX;
-	else if (uuid_equal(uuid, &uuid_null))
+	if (uuid_equal(uuid, &uuid_null))
 		return NVDIMM_CCLASS_NONE;
 
 	return NVDIMM_CCLASS_UNKNOWN;
@@ -768,20 +768,20 @@ static const guid_t *to_abstraction_guid(enum nvdimm_claim_class claim_class,
 {
 	if (claim_class == NVDIMM_CCLASS_BTT)
 		return &nvdimm_btt_guid;
-	else if (claim_class == NVDIMM_CCLASS_BTT2)
+	if (claim_class == NVDIMM_CCLASS_BTT2)
 		return &nvdimm_btt2_guid;
-	else if (claim_class == NVDIMM_CCLASS_PFN)
+	if (claim_class == NVDIMM_CCLASS_PFN)
 		return &nvdimm_pfn_guid;
-	else if (claim_class == NVDIMM_CCLASS_DAX)
+	if (claim_class == NVDIMM_CCLASS_DAX)
 		return &nvdimm_dax_guid;
-	else if (claim_class == NVDIMM_CCLASS_UNKNOWN) {
-		/*
-		 * If we're modifying a namespace for which we don't
-		 * know the claim_class, don't touch the existing guid.
-		 */
-		return target;
-	} else
+	if (claim_class == NVDIMM_CCLASS_NONE)
 		return &guid_null;
+
+	/*
+	 * If we're modifying a namespace for which we don't
+	 * know the claim_class, don't touch the existing guid.
+	 */
+	return target;
 }
 
 /* CXL labels store UUIDs instead of GUIDs for the same data */
@@ -790,20 +790,20 @@ static const uuid_t *to_abstraction_uuid(enum nvdimm_claim_class claim_class,
 {
 	if (claim_class == NVDIMM_CCLASS_BTT)
 		return &nvdimm_btt_uuid;
-	else if (claim_class == NVDIMM_CCLASS_BTT2)
+	if (claim_class == NVDIMM_CCLASS_BTT2)
 		return &nvdimm_btt2_uuid;
-	else if (claim_class == NVDIMM_CCLASS_PFN)
+	if (claim_class == NVDIMM_CCLASS_PFN)
 		return &nvdimm_pfn_uuid;
-	else if (claim_class == NVDIMM_CCLASS_DAX)
+	if (claim_class == NVDIMM_CCLASS_DAX)
 		return &nvdimm_dax_uuid;
-	else if (claim_class == NVDIMM_CCLASS_UNKNOWN) {
-		/*
-		 * If we're modifying a namespace for which we don't
-		 * know the claim_class, don't touch the existing uuid.
-		 */
-		return target;
-	} else
+	if (claim_class == NVDIMM_CCLASS_NONE)
 		return &uuid_null;
+
+	/*
+	 * If we're modifying a namespace for which we don't
+	 * know the claim_class, don't touch the existing uuid.
+	 */
+	return target;
 }
 
 static void reap_victim(struct nd_mapping *nd_mapping,
@@ -990,9 +990,7 @@ static int init_labels(struct nd_mapping *nd_mapping, int num_labels)
 		mutex_unlock(&nd_mapping->lock);
 	}
 
-	if (ndd->ns_current == -1 || ndd->ns_next == -1)
-		/* pass */;
-	else
+	if (ndd->ns_current != -1 && ndd->ns_next != -1)
 		return max(num_labels, old_num_labels);
 
 	nsindex = to_namespace_index(ndd, 0);
-- 
2.50.1

Re: [PATCH v1 1/1] libnvdimm/labels: Get rid of redundant 'else'

[Ira Weiny]

Andy Shevchenko wrote:
> In the snippets like the following
> 
> 	if (...)
> 		return / goto / break / continue ...;
> 	else
> 		...
> 
> the 'else' is redundant. Get rid of it.

What is the reason for this change?  This results in 0.6% code size
reduction.  Is that really worth this effort?

Ira

[snip]

Re: [PATCH v1 1/1] libnvdimm/labels: Get rid of redundant 'else'

[Andy Shevchenko]

On Wed, Nov 05, 2025 at 01:18:46PM -0600, Ira Weiny wrote:
> Andy Shevchenko wrote:
> > In the snippets like the following
> > 
> > 	if (...)
> > 		return / goto / break / continue ...;
> > 	else
> > 		...
> > 
> > the 'else' is redundant. Get rid of it.
> 
> What is the reason for this change?  This results in 0.6% code size
> reduction.  Is that really worth this effort?

You answered to it additionally to my point. The unneeded 'else' makes code harder to read.
Also see indentation level reduction and less LoCs overall.

I sent this patch (not as RFC or so), from my p.o.v. your Q is rhetorical :-)

P.S. I made dozens of patches like this in the past, you are the first one
questioning this, and I am glad to have a discussion started on this matter.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] libnvdimm/labels: Get rid of redundant 'else'

[Alison Schofield]

On Wed, Nov 05, 2025 at 07:37:43PM +0100, Andy Shevchenko wrote:
> In the snippets like the following
> 
> 	if (...)
> 		return / goto / break / continue ...;
> 	else
> 		...
> 
> the 'else' is redundant. Get rid of it.

The commit msg doesn't explain why the functional change in this
checkpatch cleanup is OK?

It looks like both to_abstraction_guid() and to_abstraction_uuid()
change the behavior for invalid or unexpected enum values. They use
to return &guid_null or &uuid_null, and with this patch they now
return target. That seems to remove our protection against future
enum values or corrupted enum val.


> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/nvdimm/label.c | 60 ++++++++++++++++++++----------------------
>  1 file changed, 29 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c
> index 04f4a049599a..b129f3a55a70 100644
> --- a/drivers/nvdimm/label.c
> +++ b/drivers/nvdimm/label.c

snip to the abstraction funcs

> @@ -768,20 +768,20 @@ static const guid_t *to_abstraction_guid(enum nvdimm_claim_class claim_class,
>  {
>  	if (claim_class == NVDIMM_CCLASS_BTT)
>  		return &nvdimm_btt_guid;
> -	else if (claim_class == NVDIMM_CCLASS_BTT2)
> +	if (claim_class == NVDIMM_CCLASS_BTT2)
>  		return &nvdimm_btt2_guid;
> -	else if (claim_class == NVDIMM_CCLASS_PFN)
> +	if (claim_class == NVDIMM_CCLASS_PFN)
>  		return &nvdimm_pfn_guid;
> -	else if (claim_class == NVDIMM_CCLASS_DAX)
> +	if (claim_class == NVDIMM_CCLASS_DAX)
>  		return &nvdimm_dax_guid;
> -	else if (claim_class == NVDIMM_CCLASS_UNKNOWN) {
> -		/*
> -		 * If we're modifying a namespace for which we don't
> -		 * know the claim_class, don't touch the existing guid.
> -		 */
> -		return target;
> -	} else
> +	if (claim_class == NVDIMM_CCLASS_NONE)
>  		return &guid_null;
> +
> +	/*
> +	 * If we're modifying a namespace for which we don't
> +	 * know the claim_class, don't touch the existing guid.
> +	 */
> +	return target;
>  }
>  
>  /* CXL labels store UUIDs instead of GUIDs for the same data */
> @@ -790,20 +790,20 @@ static const uuid_t *to_abstraction_uuid(enum nvdimm_claim_class claim_class,
>  {
>  	if (claim_class == NVDIMM_CCLASS_BTT)
>  		return &nvdimm_btt_uuid;
> -	else if (claim_class == NVDIMM_CCLASS_BTT2)
> +	if (claim_class == NVDIMM_CCLASS_BTT2)
>  		return &nvdimm_btt2_uuid;
> -	else if (claim_class == NVDIMM_CCLASS_PFN)
> +	if (claim_class == NVDIMM_CCLASS_PFN)
>  		return &nvdimm_pfn_uuid;
> -	else if (claim_class == NVDIMM_CCLASS_DAX)
> +	if (claim_class == NVDIMM_CCLASS_DAX)
>  		return &nvdimm_dax_uuid;
> -	else if (claim_class == NVDIMM_CCLASS_UNKNOWN) {
> -		/*
> -		 * If we're modifying a namespace for which we don't
> -		 * know the claim_class, don't touch the existing uuid.
> -		 */
> -		return target;
> -	} else
> +	if (claim_class == NVDIMM_CCLASS_NONE)
>  		return &uuid_null;
> +
> +	/*
> +	 * If we're modifying a namespace for which we don't
> +	 * know the claim_class, don't touch the existing uuid.
> +	 */
> +	return target;
>  }
>  
> 

Re: [PATCH v1 1/1] libnvdimm/labels: Get rid of redundant 'else'

[Ira Weiny]

Andy Shevchenko wrote:
> In the snippets like the following
> 
> 	if (...)
> 		return / goto / break / continue ...;
> 	else
> 		...
> 
> the 'else' is redundant. Get rid of it.
> 

I still need a why to in this commit message.

[snip]

> @@ -768,20 +768,20 @@ static const guid_t *to_abstraction_guid(enum nvdimm_claim_class claim_class,
>  {
>  	if (claim_class == NVDIMM_CCLASS_BTT)
>  		return &nvdimm_btt_guid;
> -	else if (claim_class == NVDIMM_CCLASS_BTT2)
> +	if (claim_class == NVDIMM_CCLASS_BTT2)
>  		return &nvdimm_btt2_guid;
> -	else if (claim_class == NVDIMM_CCLASS_PFN)
> +	if (claim_class == NVDIMM_CCLASS_PFN)
>  		return &nvdimm_pfn_guid;
> -	else if (claim_class == NVDIMM_CCLASS_DAX)
> +	if (claim_class == NVDIMM_CCLASS_DAX)
>  		return &nvdimm_dax_guid;
> -	else if (claim_class == NVDIMM_CCLASS_UNKNOWN) {
> -		/*
> -		 * If we're modifying a namespace for which we don't
> -		 * know the claim_class, don't touch the existing guid.
> -		 */
> -		return target;
> -	} else
> +	if (claim_class == NVDIMM_CCLASS_NONE)
>  		return &guid_null;
> +
> +	/*
> +	 * If we're modifying a namespace for which we don't
> +	 * know the claim_class, don't touch the existing guid.
> +	 */
> +	return target;

This is not an equivalent change.

>  }
>  
>  /* CXL labels store UUIDs instead of GUIDs for the same data */
> @@ -790,20 +790,20 @@ static const uuid_t *to_abstraction_uuid(enum nvdimm_claim_class claim_class,
>  {
>  	if (claim_class == NVDIMM_CCLASS_BTT)
>  		return &nvdimm_btt_uuid;
> -	else if (claim_class == NVDIMM_CCLASS_BTT2)
> +	if (claim_class == NVDIMM_CCLASS_BTT2)
>  		return &nvdimm_btt2_uuid;
> -	else if (claim_class == NVDIMM_CCLASS_PFN)
> +	if (claim_class == NVDIMM_CCLASS_PFN)
>  		return &nvdimm_pfn_uuid;
> -	else if (claim_class == NVDIMM_CCLASS_DAX)
> +	if (claim_class == NVDIMM_CCLASS_DAX)
>  		return &nvdimm_dax_uuid;
> -	else if (claim_class == NVDIMM_CCLASS_UNKNOWN) {
> -		/*
> -		 * If we're modifying a namespace for which we don't
> -		 * know the claim_class, don't touch the existing uuid.
> -		 */
> -		return target;
> -	} else
> +	if (claim_class == NVDIMM_CCLASS_NONE)
>  		return &uuid_null;
> +
> +	/*
> +	 * If we're modifying a namespace for which we don't
> +	 * know the claim_class, don't touch the existing uuid.
> +	 */
> +	return target;

This is not an equivalent change.

Ira

[snip]

Re: [PATCH v1 1/1] libnvdimm/labels: Get rid of redundant 'else'

[Andy Shevchenko]

On Thu, Nov 06, 2025 at 06:46:48PM -0600, Ira Weiny wrote:
> Andy Shevchenko wrote:
> > In the snippets like the following
> > 
> > 	if (...)
> > 		return / goto / break / continue ...;
> > 	else
> > 		...
> > 
> > the 'else' is redundant. Get rid of it.
> 
> I still need a why to in this commit message.

Sure.

[snip]

...

> > -	else if (claim_class == NVDIMM_CCLASS_UNKNOWN) {
> > -		/*
> > -		 * If we're modifying a namespace for which we don't
> > -		 * know the claim_class, don't touch the existing guid.
> > -		 */
> > -		return target;
> > -	} else
> > +	if (claim_class == NVDIMM_CCLASS_NONE)
> >  		return &guid_null;
> > +
> > +	/*
> > +	 * If we're modifying a namespace for which we don't
> > +	 * know the claim_class, don't touch the existing guid.
> > +	 */
> > +	return target;
> 
> This is not an equivalent change.

It's (okau. almost. later on that). The parameter to the function is enum and
the listed values of the enum is all there. The problematic part can be if
somebody supplies an arbitrary value here. Yes, in such a case it will change
behaviour and I think it is correct in my case as UNKNOWN is unknown, and NONE
is actually well known UUID.

...

> > -	else if (claim_class == NVDIMM_CCLASS_UNKNOWN) {
> > -		/*
> > -		 * If we're modifying a namespace for which we don't
> > -		 * know the claim_class, don't touch the existing uuid.
> > -		 */
> > -		return target;
> > -	} else
> > +	if (claim_class == NVDIMM_CCLASS_NONE)
> >  		return &uuid_null;
> > +
> > +	/*
> > +	 * If we're modifying a namespace for which we don't
> > +	 * know the claim_class, don't touch the existing uuid.
> > +	 */
> > +	return target;
> 
> This is not an equivalent change.

Same explanation as above. I'll put it into the commit message.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] libnvdimm/labels: Get rid of redundant 'else'

[Ira Weiny]

Andy Shevchenko wrote:
> On Thu, Nov 06, 2025 at 06:46:48PM -0600, Ira Weiny wrote:
> > Andy Shevchenko wrote:

[snip]

> > > -	else if (claim_class == NVDIMM_CCLASS_UNKNOWN) {
> > > -		/*
> > > -		 * If we're modifying a namespace for which we don't
> > > -		 * know the claim_class, don't touch the existing guid.
> > > -		 */
> > > -		return target;
> > > -	} else
> > > +	if (claim_class == NVDIMM_CCLASS_NONE)
> > >  		return &guid_null;
> > > +
> > > +	/*
> > > +	 * If we're modifying a namespace for which we don't
> > > +	 * know the claim_class, don't touch the existing guid.
> > > +	 */
> > > +	return target;
> > 
> > This is not an equivalent change.
> 
> It's (okau. almost. later on that). The parameter to the function is enum and
> the listed values of the enum is all there.

True.

> The problematic part can be if
> somebody supplies an arbitrary value here. Yes, in such a case it will change
> behaviour and I think it is correct in my case as UNKNOWN is unknown, and NONE
> is actually well known UUID.

Yea putting this in the commit message but more importantly knowing you
looked through the logic of how claim class is used is what I'm looking
for.

Thanks,
Ira

[snip]

Re: [PATCH v1 1/1] libnvdimm/labels: Get rid of redundant 'else'

[Alison Schofield]

On Fri, Nov 07, 2025 at 10:23:32AM -0600, Ira Weiny wrote:
> 
> Yea putting this in the commit message but more importantly knowing you
> looked through the logic of how claim class is used is what I'm looking
> for.
> 
> Thanks,
> Ira
>

Hi Ira,

Coming back around to this patch after a few days, after initially
commenting on the unexplained behavior change, I realize a better
response would have been a simple NAK.

This patch demonstrates why style-only cleanups are generally discouraged
outside of drivers/staging. It creates code churn without fixing bugs
or adding functionality, the changes aren't justified in the commit
message, it adds risk, and consumes limited reviewer and maintainer
bandwidth.

To recoup value from the time already spent on this, I suggest using
this opportunity to set a clear position and precedent, like:

	"Style cleanups are not welcomed in the NVDIMM subsystem unless
	they're part of a fix or a patch series that includes substantive
	changes to the same code area."

FWIW, if folks are looking to dive into this code, there is a patchset
in review here[1] that adds new functionality to this area. Reviews,
including style reviews, are welcomed.

Regardless of a commit message update or a change to the code, this
one is a NAK from me.

--Alison

[1] https://lore.kernel.org/nvdimm/20250917132940.1566437-1-s.neeraj@samsung.com/

Re: [PATCH v1 1/1] libnvdimm/labels: Get rid of redundant 'else'

[Andy Shevchenko]

On Sun, Nov 09, 2025 at 02:11:54PM -0800, Alison Schofield wrote:
> On Fri, Nov 07, 2025 at 10:23:32AM -0600, Ira Weiny wrote:
> > 
> > Yea putting this in the commit message but more importantly knowing you
> > looked through the logic of how claim class is used is what I'm looking
> > for.
> 
> Coming back around to this patch after a few days, after initially
> commenting on the unexplained behavior change, I realize a better
> response would have been a simple NAK.
> 
> This patch demonstrates why style-only cleanups are generally discouraged
> outside of drivers/staging. It creates code churn without fixing bugs
> or adding functionality, the changes aren't justified in the commit
> message, it adds risk, and consumes limited reviewer and maintainer
> bandwidth.
> 
> To recoup value from the time already spent on this, I suggest using
> this opportunity to set a clear position and precedent, like:
> 
> 	"Style cleanups are not welcomed in the NVDIMM subsystem unless
> 	they're part of a fix or a patch series that includes substantive
> 	changes to the same code area."

Let's rotten it with the old APIs and style then :-)

I have heard you and I won't try even bring any new patch in this subsystem, thanks.

> FWIW, if folks are looking to dive into this code, there is a patchset
> in review here[1] that adds new functionality to this area. Reviews,
> including style reviews, are welcomed.
> 
> Regardless of a commit message update or a change to the code, this
> one is a NAK from me.
> 
> [1] https://lore.kernel.org/nvdimm/20250917132940.1566437-1-s.neeraj@samsung.com/

-- 
With Best Regards,
Andy Shevchenko