Patchwork [2/5] UBI: fastmap: kill junk newlines and add a TODO about that

login
register
mail settings
Submitter Artem Bityutskiy
Date June 5, 2012, 3:11 p.m.
Message ID <1338909119-5188-3-git-send-email-dedekind1@gmail.com>
Download mbox | patch
Permalink /patch/163094/
State New
Headers show

Comments

Artem Bityutskiy - June 5, 2012, 3:11 p.m.
From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 drivers/mtd/ubi/fastmap.c |   45 ++++++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 21 deletions(-)
Richard Weinberger - June 6, 2012, 9:30 p.m.
Am 05.06.2012 17:11, schrieb Artem Bityutskiy:
> +		/* TODO: nitpick, sorry for being annoying, but this should not
> +		 * be difficult to fix. I find it very irritating when there
> +		 * are useless blank lines like this - they only make less code
> +		 * fit my screen and make large functions even larger.
> +		 *
> +		 * Why we use blank lines inside a single function? To make
> +		 * code more readable. How we make it more readable? By
> +		 * separating logically different blocks of code with a
> +		 * newline.
> +		 *
> +		 * What is the perbose of these newlines before goto's? This is
> +		 * a single piece of error-handling code - you assing the
> +		 * return value and go to the further error processing. These
> +		 * newlines serve no purpose just blow the lines number in the
> +		 * code. Could we please kill them globally?
> +		 *
> +		 * I am again sorry - probably this is not completely
> +		 * technical, but I gave the explanation why these newlines are
> +		 * annying. And as a person who spent a lot of personal
> +		 * (non-paid) time maintaining this code and who will keep
> +		 * doing this - I think I have right to require to do such
> +		 * cosmetic things :-) If you do not agree with my reasoning,
> +		 * may be this is better - you'll keep me happier :-) */
>  		goto out;

Your and mine coding style are quite different. :-)
I *hate* it when statements like goto and return are glued to a code block.
E.g.

a = 1 + b;
res = c + a;

return res;

vs:

a = 1 + b;
res = c + a;
return res;

Anyway, I'll change this to match the UBI style.

Thanks,
//richard

P.s: Can we please have a ubi_checkpatch.pl? *SCNR*
Artem Bityutskiy - June 6, 2012, 11:29 p.m.
On Wed, 2012-06-06 at 23:30 +0200, Richard Weinberger wrote:
> a = 1 + b;
> res = c + a;
> 
> return res;
> 
> vs:
> 
> a = 1 + b;
> res = c + a;
> return res;

Sorry, but I think the above example is not quite suitable because it
looks too general, while I was asking only about a specific thing, not
for any return or goto.

ret = func();
if (ret) {
	free(x);

	goto out;
}

Patch

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 09629c2..b2ee872 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -751,7 +751,6 @@  int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 	fmsb = kmalloc(sizeof(*fmsb), GFP_KERNEL);
 	if (!fmsb) {
 		ret = -ENOMEM;
-
 		goto out;
 	}
 
@@ -764,9 +763,7 @@  int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 		/* TODO: what are the error codes > 0 ? Why is this check? */
 		if (ret > 0)
 			ret = UBI_BAD_FASTMAP;
-
 		kfree(fmsb);
-
 		goto out;
 	}
 
@@ -784,7 +781,6 @@  int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 		ubi_err("super block magic does not match");
 		ret = UBI_BAD_FASTMAP;
 		kfree(fmsb);
-
 		goto out;
 	}
 
@@ -792,17 +788,14 @@  int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 		ubi_err("unknown fastmap format version!");
 		ret = UBI_BAD_FASTMAP;
 		kfree(fmsb);
-
 		goto out;
 	}
 
 	used_blocks = be32_to_cpu(fmsb->used_blocks);
-
 	if (used_blocks > UBI_FM_MAX_BLOCKS || used_blocks < 1) {
 		ubi_err("number of fastmap blocks is invalid");
 		ret = UBI_BAD_FASTMAP;
 		kfree(fmsb);
-
 		goto out;
 	}
 
@@ -812,7 +805,6 @@  int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 	if (!fm_raw) {
 		ret = -ENOMEM;
 		kfree(fmsb);
-
 		goto out;
 	}
 
@@ -820,7 +812,6 @@  int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 	if (!ech) {
 		ret = -ENOMEM;
 		kfree(fmsb);
-
 		goto free_raw;
 	}
 
@@ -829,7 +820,6 @@  int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 		ret = -ENOMEM;
 		kfree(fmsb);
 		kfree(ech);
-
 		goto free_raw;
 	}
 
@@ -839,7 +829,6 @@  int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 		if (ubi_io_is_bad(ubi, pnum)) {
 			ret = UBI_BAD_FASTMAP;
 			kfree(fmsb);
-
 			goto free_hdr;
 		}
 
@@ -850,7 +839,6 @@  int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 			if (ret > 0)
 				ret = UBI_BAD_FASTMAP;
 			kfree(fmsb);
-
 			goto free_hdr;
 		}
 
@@ -860,7 +848,6 @@  int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 		if (be32_to_cpu(ech->image_seq) != ubi->image_seq) {
 			ret = UBI_BAD_FASTMAP;
 			kfree(fmsb);
-
 			goto free_hdr;
 		}
 
@@ -871,7 +858,6 @@  int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 			if (ret > 0)
 				ret = UBI_BAD_FASTMAP;
 			kfree(fmsb);
-
 			goto free_hdr;
 		}
 
@@ -886,7 +872,6 @@  int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 			if (be32_to_cpu(vh->vol_id) != UBI_FM_DATA_VOLUME_ID) {
 				ret = UBI_BAD_FASTMAP;
 				kfree(fmsb);
-
 				goto free_hdr;
 			}
 		}
@@ -903,7 +888,6 @@  int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 			if (ret > 0)
 				ret = UBI_BAD_FASTMAP;
 			kfree(fmsb);
-
 			goto free_hdr;
 		}
 	}
@@ -927,14 +911,12 @@  int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 	if (ret) {
 		if (ret > 0)
 			ret = UBI_BAD_FASTMAP;
-
 		goto free_hdr;
 	}
 
 	ai->fm = kzalloc(sizeof(*ai->fm), GFP_KERNEL);
 	if (!ai->fm) {
 		ret = -ENOMEM;
-
 		goto free_hdr;
 	}
 
@@ -952,12 +934,11 @@  int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 			kfree(ai->fm);
 			ai->fm = NULL;
 			ret = -ENOMEM;
-
 			goto free_hdr;
 		}
+
 		e->pnum = be32_to_cpu(fmsb->block_loc[i]);
 		e->ec = be32_to_cpu(fmsb->block_ec[i]);
-
 		ai->fm->e[i] = e;
 	}
 
@@ -969,7 +950,6 @@  free_raw:
 out:
 	if (ret == UBI_BAD_FASTMAP)
 		ubi_err("Attach by fastmap failed, doing a full scan!");
-
 	return ret;
 }
 
@@ -1005,6 +985,29 @@  static int ubi_write_fastmap(struct ubi_device *ubi,
 	if (!fm_raw) {
 		ret = -ENOMEM;
 
+		/* TODO: nitpick, sorry for being annoying, but this should not
+		 * be difficult to fix. I find it very irritating when there
+		 * are useless blank lines like this - they only make less code
+		 * fit my screen and make large functions even larger.
+		 *
+		 * Why we use blank lines inside a single function? To make
+		 * code more readable. How we make it more readable? By
+		 * separating logically different blocks of code with a
+		 * newline.
+		 *
+		 * What is the perbose of these newlines before goto's? This is
+		 * a single piece of error-handling code - you assing the
+		 * return value and go to the further error processing. These
+		 * newlines serve no purpose just blow the lines number in the
+		 * code. Could we please kill them globally?
+		 *
+		 * I am again sorry - probably this is not completely
+		 * technical, but I gave the explanation why these newlines are
+		 * annying. And as a person who spent a lot of personal
+		 * (non-paid) time maintaining this code and who will keep
+		 * doing this - I think I have right to require to do such
+		 * cosmetic things :-) If you do not agree with my reasoning,
+		 * may be this is better - you'll keep me happier :-) */
 		goto out;
 	}