diff mbox series

[11/13] nvdimm: Use more common logic testing styles and bare ; positions

Message ID d6aa5b66936f2e0f132e893e10494aae6b74e886.1568256708.git.joe@perches.com (mailing list archive)
State New, archived
Headers show
Series nvdimm: Use more common kernel coding style | expand

Commit Message

Joe Perches Sept. 12, 2019, 2:54 a.m. UTC 
Avoid using uncommon logic testing styles to make the code a
bit more like other kernel code.

e.g.:
	if (foo) {
		;
	} else {
		<code>
	}

is typically written

	if (!foo) {
		<code>
	}

Also put bare semicolons before the comment not after the comment

e.g.:

	if (foo) {
		/* comment */;
	} else if (bar) {
		<code>
	} else {
		baz;
	}

is typically written

	if (foo) {
		;	/* comment */
	} else if (bar) {
		<code>
	} else {
		baz;
	}

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/nvdimm/claim.c          |  4 +---
 drivers/nvdimm/dimm_devs.c      | 11 ++++------
 drivers/nvdimm/label.c          |  4 +---
 drivers/nvdimm/namespace_devs.c | 46 +++++++++++++++++++----------------------
 drivers/nvdimm/region_devs.c    |  4 +---
 5 files changed, 28 insertions(+), 41 deletions(-)

Comments

Verma, Vishal L Sept. 12, 2019, 3:52 a.m. UTC | #1 
On Wed, 2019-09-11 at 19:54 -0700, Joe Perches wrote:
> Avoid using uncommon logic testing styles to make the code a
> bit more like other kernel code.
> 
> e.g.:
> 	if (foo) {
> 		;
> 	} else {
> 		<code>
> 	}
> 
> is typically written
> 
> 	if (!foo) {
> 		<code>
> 	}
> 

A lot of times the excessive inversions seem to result in a net loss of
readability - e.g.:

<snip>

> diff --git a/drivers/nvdimm/region_devs.c
> b/drivers/nvdimm/region_devs.c
> index 65df07481909..6861e0997d21 100644
> --- a/drivers/nvdimm/region_devs.c
> +++ b/drivers/nvdimm/region_devs.c
> @@ -320,9 +320,7 @@ static ssize_t set_cookie_show(struct device *dev,
>  	struct nd_interleave_set *nd_set = nd_region->nd_set;
>  	ssize_t rc = 0;
>  
> -	if (is_memory(dev) && nd_set)
> -		/* pass, should be precluded by region_visible */;

For one, the comment is lost

> -	else
> +	if (!(is_memory(dev) && nd_set))

And it takes a moment to resolve between things such as:

	if (!(A && B))
	  vs.
	if (!(A) && B)

And this is especially true if 'A' and 'B' are longer function calls,
split over multiple lines, or are themselves compound 'sections'.

I'm not opposed to /all/ such transformations -- for example, the ones
where the logical inversion can be 'consumed' by toggling a comparision
operator, as you have a few times in this patch, don't sacrifice any
readibility, and perhaps even improve it. 

>  		return -ENXIO;
>  
>  	/*
diff mbox series

Patch

diff --git a/drivers/nvdimm/claim.c b/drivers/nvdimm/claim.c
index 3732925aadb8..244631f5308c 100644
--- a/drivers/nvdimm/claim.c
+++ b/drivers/nvdimm/claim.c
@@ -149,9 +149,7 @@  ssize_t nd_namespace_store(struct device *dev,
 		return -ENOMEM;
 	strim(name);
 
-	if (strncmp(name, "namespace", 9) == 0 || strcmp(name, "") == 0) {
-		/* pass */;
-	} else {
+	if (!(strncmp(name, "namespace", 9) == 0 || strcmp(name, "") == 0)) {
 		len = -EINVAL;
 		goto out;
 	}
diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
index 4df85dd72682..cac62bb726bb 100644
--- a/drivers/nvdimm/dimm_devs.c
+++ b/drivers/nvdimm/dimm_devs.c
@@ -593,13 +593,10 @@  int alias_dpa_busy(struct device *dev, void *data)
 	 * looking to validate against PMEM aliasing collision rules
 	 * (i.e. BLK is allocated after all aliased PMEM).
 	 */
-	if (info->res) {
-		if (info->res->start >= nd_mapping->start &&
-		    info->res->start < map_end)
-			/* pass */;
-		else
-			return 0;
-	}
+	if (info->res &&
+	    (info->res->start < nd_mapping->start ||
+	     info->res->start >= map_end))
+		return 0;
 
 retry:
 	/*
diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c
index e4632dbebead..ae466c6faa90 100644
--- a/drivers/nvdimm/label.c
+++ b/drivers/nvdimm/label.c
@@ -1180,9 +1180,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);
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index 8c75ef84bad7..7a16340f9853 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -162,7 +162,7 @@  unsigned int pmem_sector_size(struct nd_namespace_common *ndns)
 
 		nspm = to_nd_namespace_pmem(&ndns->dev);
 		if (nspm->lbasize == 0 || nspm->lbasize == 512)
-			/* default */;
+			;	/* default */
 		else if (nspm->lbasize == 4096)
 			return 4096;
 		else
@@ -387,7 +387,7 @@  static int nd_namespace_label_update(struct nd_region *nd_region,
 		resource_size_t size = resource_size(&nspm->nsio.res);
 
 		if (size == 0 && nspm->uuid)
-			/* delete allocation */;
+			;	/* delete allocation */
 		else if (!nspm->uuid)
 			return 0;
 
@@ -398,7 +398,7 @@  static int nd_namespace_label_update(struct nd_region *nd_region,
 		resource_size_t size = nd_namespace_blk_size(nsblk);
 
 		if (size == 0 && nsblk->uuid)
-			/* delete allocation */;
+			;	/* delete allocation */
 		else if (!nsblk->uuid || !nsblk->lbasize)
 			return 0;
 
@@ -1900,10 +1900,8 @@  static int select_pmem_id(struct nd_region *nd_region, u8 *pmem_id)
 		hw_end = hw_start + nd_mapping->size;
 		pmem_start = __le64_to_cpu(nd_label->dpa);
 		pmem_end = pmem_start + __le64_to_cpu(nd_label->rawsize);
-		if (pmem_start >= hw_start && pmem_start < hw_end &&
-		    pmem_end <= hw_end && pmem_end > hw_start) {
-			/* pass */;
-		} else {
+		if (!(pmem_start >= hw_start && pmem_start < hw_end &&
+		      pmem_end <= hw_end && pmem_end > hw_start)) {
 			dev_dbg(&nd_region->dev, "%s invalid label for %pUb\n",
 				dev_name(ndd->dev), nd_label->uuid);
 			return -EINVAL;
@@ -2326,15 +2324,12 @@  static struct device **scan_labels(struct nd_region *nd_region)
 	list_for_each_entry_safe(label_ent, e, &nd_mapping->labels, list) {
 		struct nd_namespace_label *nd_label = label_ent->label;
 		struct device **__devs;
-		u32 flags;
+		bool localflag;
 
 		if (!nd_label)
 			continue;
-		flags = __le32_to_cpu(nd_label->flags);
-		if (is_nd_blk(&nd_region->dev)
-		    == !!(flags & NSLABEL_FLAG_LOCAL))
-			/* pass, region matches label type */;
-		else
+		localflag = __le32_to_cpu(nd_label->flags) & NSLABEL_FLAG_LOCAL;
+		if (is_nd_blk(&nd_region->dev) != localflag)
 			continue;
 
 		/* skip labels that describe extents outside of the region */
@@ -2494,19 +2489,20 @@  static int init_active_labels(struct nd_region *nd_region)
 		 * the region from being activated.
 		 */
 		if (!ndd) {
-			if (test_bit(NDD_LOCKED, &nvdimm->flags))
-				/* fail, label data may be unreadable */;
-			else if (test_bit(NDD_ALIASING, &nvdimm->flags))
-				/* fail, labels needed to disambiguate dpa */;
-			else
-				return 0;
-
-			dev_err(&nd_region->dev, "%s: is %s, failing probe\n",
-				dev_name(&nd_mapping->nvdimm->dev),
-				test_bit(NDD_LOCKED, &nvdimm->flags)
-				? "locked" : "disabled");
-			return -ENXIO;
+			if (test_bit(NDD_LOCKED, &nvdimm->flags) ||
+					/* label data may be unreadable */
+			    test_bit(NDD_ALIASING, &nvdimm->flags)) {
+					/* labels needed to disambiguate dpa */
+
+				dev_err(&nd_region->dev, "%s: is %s, failing probe\n",
+					dev_name(&nd_mapping->nvdimm->dev),
+					test_bit(NDD_LOCKED, &nvdimm->flags)
+					? "locked" : "disabled");
+				return -ENXIO;
+			}
+			return 0;
 		}
+
 		nd_mapping->ndd = ndd;
 		atomic_inc(&nvdimm->busy);
 		get_ndd(ndd);
diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index 65df07481909..6861e0997d21 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -320,9 +320,7 @@  static ssize_t set_cookie_show(struct device *dev,
 	struct nd_interleave_set *nd_set = nd_region->nd_set;
 	ssize_t rc = 0;
 
-	if (is_memory(dev) && nd_set)
-		/* pass, should be precluded by region_visible */;
-	else
+	if (!(is_memory(dev) && nd_set))
 		return -ENXIO;
 
 	/*