[patch] checkpatch: complain about GW-BASIC style label names

[patch] checkpatch: complain about GW-BASIC style label names

[Dan Carpenter]

GW-BASIC style label names are quite common.  This generates a warning
like:

	WARNING: bad label name
	#795: FILE: drivers/ata/pata_mpc52xx.c:795:
	+ err2:

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 89b1df4..ee3fa30 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4023,6 +4023,16 @@ sub process {
 			}
 		}
 
+#avoid GW-BASIC style label names
+		if ($line=~/^\+\s*(err|error|fail|out)[0-9]+:/) {
+			if (WARN("LABEL_NAME",
+				 "bad label name\n" . $herecurr) &&
+			    $fix) {
+				$fixed[$fixlinenr] =~
+				    s/^(.)\s+/$1/;
+			}
+		}
+
 # return is not a function
 		if (defined($stat) && $stat =~ /^.\s*return(\s*)\(/s) {
 			my $spacing = $1;

Re: [patch] checkpatch: complain about GW-BASIC style label names

[Joe Perches]

On Thu, 2015-05-07 at 14:21 +0300, Dan Carpenter wrote:
> GW-BASIC style label names are quite common.  This generates a warning
> like:
> 
> 	WARNING: bad label name
> 	#795: FILE: drivers/ata/pata_mpc52xx.c:795:
> 	+ err2:

Hey Dan.

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -4023,6 +4023,16 @@ sub process {
>  			}
>  		}
>  
> +#avoid GW-BASIC style label names
> +		if ($line=~/^\+\s*(err|error|fail|out)[0-9]+:/) {

Labels aren't always only lower case.

This may have false positives with ?: uses like
	a = foo ?
		err1:err2;

> +			if (WARN("LABEL_NAME",
> +				 "bad label name\n" . $herecurr) &&
> +			    $fix) {
> +				$fixed[$fixlinenr] =~
> +				    s/^(.)\s+/$1/;
> +			}
> +		}

There already is a $fix option in the INDENTED_LABEL test
above this one and isn't needed or wanted here.

It may be better to use a message like:
	"Prefer functionally descriptive label naming (ie: label<why>:)\n"

Re: [patch] checkpatch: complain about GW-BASIC style label names

[Dan Carpenter]

On Thu, May 07, 2015 at 06:47:51AM -0700, Joe Perches wrote:
> > +#avoid GW-BASIC style label names
> > +		if ($line=~/^\+\s*(err|error|fail|out)[0-9]+:/) {
> 
> Labels aren't always only lower case.

Fair enough.  I'll test again with:

		if ($line=~/^\+\s*(err|error|fail|out)[0-9]+:/i) {

> 
> This may have false positives with ?: uses like
> 	a = foo ?
> 		err1:err2;
> 

There are no false positives.  :)

$ grep -A2 "bad label name" errs | grep ^+ | cut -b 2- | perl -ne 's/( |\t)//g; print' | cut -d : -f 1 | sort | uniq -c | sort -rn | less
    249 err1
    209 err2
    168 out1
    150 out2
    149 fail1
    132 fail2
     83 fail3
     83 err3
     78 err0
     73 out3
     70 error1
     67 error2
     48 fail0
     43 err4
     42 error0
     33 error3
     31 out4
     29 fail4
     23 out0
     22 err5
     21 error4
     14 fail5
     12 out5
     11 error5
      9 err6
      8 out6
      7 error6
      6 out7
      6 fail6
      5 out8
      5 err7
      3 error7
      2 out9
      2 error9
      2 error8
      2 err8
      2 err05
      1 out10
      1 fail9
      1 fail8
      1 fail7
      1 error10
      1 err06
      1 err04
      1 err03
      1 err01

> > +			ir (WARN("LABEL_NAME",
> > +				 "bad label name\n" . $herecurr) &&
> > +			    $fix) {
> > +				$fixed[$fixlinenr] =~
> > +				    s/^(.)\s+/$1/;
> > +			}
> > +		}
> 
> There already is a $fix option in the INDENTED_LABEL test
> above this one and isn't needed or wanted here.
> 

To be honest, I'm crap at Perl.  Give me something to cut and paste?

> It may be better to use a message like:
> 	"Prefer functionally descriptive label naming (ie: label<why>:)\n"
> 

These things are always the trickiest bit.  I think of why as the goto
location and not the label location.

WARNING: Prefer a more descriptive label.  What does the label do?\n

It's also a bit crap because labels don't do anything.

regards,
dan carpenter

Re: [patch] checkpatch: complain about GW-BASIC style label names

[Joe Perches]

On Thu, 2015-05-07 at 22:42 +0300, Dan Carpenter wrote:
> On Thu, May 07, 2015 at 06:47:51AM -0700, Joe Perches wrote:
> > > +#avoid GW-BASIC style label names
[]
> To be honest, I'm crap at Perl.

Me too.

> Give me something to cut and paste?

[maybe below...]

> > It may be better to use a message like:
> > 	"Prefer functionally descriptive label naming (ie: label<why>:)\n"
> > 
> 
> These things are always the trickiest bit.  I think of why as the goto
> location and not the label location.
> 
> WARNING: Prefer a more descriptive label.  What does the label do?\n
> It's also a bit crap because labels don't do anything.

Maybe something like:

# check for label names likely used in a numeric sequence of labels
		if ($line =~ /^.\s*((?:err|error|fail|out)[0-9+])\s*:/i ||
		    $line =~ /\bgoto\s+((?:err|error|fail|out)[0-9+])\s*[:;,]/) {
			my $label = "This label";
			if (defined $1) {
				$label = $1;
			} else {
				$label = $2;
			}
			WARN("BAD_LABEL_NAME",
			     "$label isn't informative - prefer descriptive gotos and labels\n" . $herecurr);
		}

Re: [patch] checkpatch: complain about GW-BASIC style label names

[Joe Perches]

On Thu, 2015-05-07 at 13:17 -0700, Joe Perches wrote:
> # check for label names likely used in a numeric sequence of labels
> 		if ($line =~ /^.\s*((?:err|error|fail|out)[0-9+])\s*:/i ||
> 		    $line =~ /\bgoto\s+((?:err|error|fail|out)[0-9+])\s*[:;,]/) {

Meh.  I told you about my perl-fu...

That + should be outside the close bracket and it's probably
better that this use a funny (?i) use inside the capture group
so that the goto is lower case only, but the label can be
lower, upper and mixed case.

		if ($line =~ /^.\s*((?i)(?:err|error|fail|out)[0-9]+)\s*:/ ||
 		    $line =~ /\bgoto\s+((?i)(?:err|error|fail|out)[0-9]+)\s*[:;,]/) {

[patch v2] checkpatch: complain about GW-BASIC style label names

[Dan Carpenter]

GW-BASIC style label names are annoying so we can warn about that in
checkpatch.  The warnings look like:

	WARNING: 'fail2' isn't informative - prefer descriptive label names
	#267: FILE: ./sound/ppc/beep.c:267:
	+ fail2:        snd_ctl_remove(chip->card, beep_ctl);

This generates slightly under 2000 new warnings.  None of them are
false positives.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: I don't want to warn about gotos...

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 89b1df4..c9e4c5b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4023,6 +4023,16 @@ sub process {
 			}
 		}
 
+#avoid GW-BASIC style label names
+		if ($line =~ /^.\s*((?i)(?:err|error|fail|out)[0-9]+)\s*:/) {
+			my $label = "This label";
+			if (defined $1) {
+				$label = $1;
+			}
+			WARN("BAD_LABEL_NAME",
+				 "'$label' isn't informative - prefer descriptive label names\n" . $herecurr);
+		}
+
 # return is not a function
 		if (defined($stat) && $stat =~ /^.\s*return(\s*)\(/s) {
 			my $spacing = $1;

Re: [patch v2] checkpatch: complain about GW-BASIC style label names

[David Sterba]

On Wed, May 13, 2015 at 03:37:12PM +0300, Dan Carpenter wrote:
> GW-BASIC style label names are annoying so we can warn about that in
> checkpatch.  The warnings look like:
> 
> 	WARNING: 'fail2' isn't informative - prefer descriptive label names
> 	#267: FILE: ./sound/ppc/beep.c:267:
> 	+ fail2:        snd_ctl_remove(chip->card, beep_ctl);
> 
> This generates slightly under 2000 new warnings.  None of them are
> false positives.

Please whitelist fs/btrfs/* from this type of checkpatch warning. Using
the out: or err: is a very common pattern and nobody has complained so
far, besides that I find it descriptive enough in most cases. Where it
needs more description out_something is being used.

My understanding is that developers that work on a particular subsystems
like to see the same code patterns, whitespace style etc.. Local
variations are possible and more or less tolerated.

The checkpatch tool should IMHO catch the global style violations. Once
it starts to interfere with local style variations it's becommig less
useful as it requires extra rounds to tell people "please change it like
that and don't listen to checkpatch". And pointing out minor style or
whitespace issues mismatching the preferences is not what the reviewers
should spend their time on.

This kind of change does not help us, but I guess this is not the first
you hear this.

Re: [patch v2] checkpatch: complain about GW-BASIC style label names

[Dan Carpenter]

You misunderstand.  Although I am famous for hating out: labels, I would
not introduce a checkpatch warning to complain about it.  This only
complains about GW-BASIC labels.

out3:
	kfree(foo);
out2:
	kfree(bar);
out:
	kfree(baz);

GW-BASIC label suck because they are meaningless and lazy and, if you
introduce a new warning in the middle, then you have to rename them all.
In btrfs this only complains about the following two sections of code:

fs/btrfs/compression.c
   732  
   733  fail2:
   734          while (faili >= 0) {
   735                  __free_page(cb->compressed_pages[faili]);
   736                  faili--;
   737          }
   738  
   739          kfree(cb->compressed_pages);
   740  fail1:
   741          kfree(cb);
   742  out:
   743          free_extent_map(em);
   744          return ret;
   745  }

fs/btrfs/sysfs.c
   742  
   743          return 0;
   744  out2:
   745          debugfs_remove_recursive(btrfs_debugfs_root_dentry);
   746  out1:
   747          kset_unregister(btrfs_kset);
   748  
   749          return ret;
   750  }

regards,
dan carpenter

Re: [patch v2] checkpatch: complain about GW-BASIC style label names

[One Thousand Gnomes]

On Wed, 13 May 2015 15:16:13 +0200
David Sterba <dsterba@suse.cz> wrote:

> On Wed, May 13, 2015 at 03:37:12PM +0300, Dan Carpenter wrote:
> > GW-BASIC style label names are annoying so we can warn about that in
> > checkpatch.  The warnings look like:
> > 
> > 	WARNING: 'fail2' isn't informative - prefer descriptive label names
> > 	#267: FILE: ./sound/ppc/beep.c:267:
> > 	+ fail2:        snd_ctl_remove(chip->card, beep_ctl);
> > 
> > This generates slightly under 2000 new warnings.  None of them are
> > false positives.
> 
> Please whitelist fs/btrfs/* from this type of checkpatch warning.

If you could whitelist the rest of the kernel too that would also be
useful.

There's nothing wrong with driver code that ends


fail_3:
	xxx
fail_2:
	yyy
fail_1:

	blah
	return;

if anything it makes it very clear which level of unravelling on error is
occurring and at a glance enables you to see that the error handling is
ordered properly.

Re: [patch v2] checkpatch: complain about GW-BASIC style label names

[Al Viro]

On Wed, May 13, 2015 at 03:37:12PM +0300, Dan Carpenter wrote:
> GW-BASIC style label names are annoying so we can warn about that in
> checkpatch.  The warnings look like:
> 
> 	WARNING: 'fail2' isn't informative - prefer descriptive label names
> 	#267: FILE: ./sound/ppc/beep.c:267:
> 	+ fail2:        snd_ctl_remove(chip->card, beep_ctl);
> 
> This generates slightly under 2000 new warnings.  None of them are
> false positives.

And all of them are complete garbage.  Please, stop forcing your personal
preferences upon the rest of us.

Any "fixes" of that sort in fs/*.c will be passed straight to /dev/null.

Re: [patch v2] checkpatch: complain about GW-BASIC style label names

[Dan Carpenter]

It's weird that you would defend GW-BASIC label names because you
wouldn't defend code which does:

	int var1, var2, var4;

Naming labels is useful.

	goto error9;
	goto err_cleanup_sysfs1;

The second one is more clear.  But it's better to look at it in context:

drivers/hid/hid-picolcd_core.c
   584          error = device_create_file(&hdev->dev, &dev_attr_operation_mode_delay);
   585          if (error) {
   586                  hid_err(hdev, "failed to create sysfs attributes\n");
   587                  goto err_cleanup_hid_ll;
   588          }
   589  
   590          error = device_create_file(&hdev->dev, &dev_attr_operation_mode);
   591          if (error) {
   592                  hid_err(hdev, "failed to create sysfs attributes\n");
   593                  goto err_cleanup_sysfs1;
   594          }

Without scrolling down, you already know that the error handling is
going to uncreate the dev_attr_operation_mode_delay files so it's
correct.

drivers/infiniband/core/mad.c
  2977          snprintf(name, sizeof name, "ib_mad%d", port_num);
  2978          port_priv->wq = create_singlethread_workqueue(name);
  2979          if (!port_priv->wq) {
  2980                  ret = -ENOMEM;
  2981                  goto error8;
  2982          }
  2983          INIT_WORK(&port_priv->work, ib_mad_completion_handler);
  2984  
  2985          spin_lock_irqsave(&ib_mad_port_list_lock, flags);
  2986          list_add_tail(&port_priv->port_list, &ib_mad_port_list);
  2987          spin_unlock_irqrestore(&ib_mad_port_list_lock, flags);
  2988  
  2989          ret = ib_mad_port_start(port_priv);
  2990          if (ret) {
  2991                  dev_err(&device->dev, "Couldn't start port\n");
  2992                  goto error9;
  2993          }

Hopefully, it undoes the list_add_tail() but the error9 isn't clear.

Here are the labels:

drivers/infiniband/core/mad.c
  2995          return 0;
  2996  
  2997  error9:
  2998          spin_lock_irqsave(&ib_mad_port_list_lock, flags);
  2999          list_del_init(&port_priv->port_list);
  3000          spin_unlock_irqrestore(&ib_mad_port_list_lock, flags);
  3001  
  3002          destroy_workqueue(port_priv->wq);
  3003  error8:
  3004          destroy_mad_qp(&port_priv->qp_info[1]);
  3005  error7:
  3006          destroy_mad_qp(&port_priv->qp_info[0]);
  3007  error6:
  3008          ib_dereg_mr(port_priv->mr);
  3009  error5:
  3010          ib_dealloc_pd(port_priv->pd);
  3011  error4:
  3012          ib_destroy_cq(port_priv->cq);
  3013          cleanup_recv_queue(&port_priv->qp_info[1]);
  3014          cleanup_recv_queue(&port_priv->qp_info[0]);
  3015  error3:
  3016          kfree(port_priv);
  3017  
  3018          return ret;
  3019  }

So ok, it is correct.  But does error8 do what was intended?  You can't
tell without scrolling back.  Also this is the first example which I
chose at random but it makes me happy that error2 and error1 are
missing.

Here is the labels with meaningful names:

drivers/hid/hid-picolcd_core.c
   606  err_cleanup_sysfs2:
   607          device_remove_file(&hdev->dev, &dev_attr_operation_mode);
   608  err_cleanup_sysfs1:
   609          device_remove_file(&hdev->dev, &dev_attr_operation_mode_delay);
   610  err_cleanup_hid_ll:
   611          hid_hw_close(hdev);
   612  err_cleanup_hid_hw:
   613          hid_hw_stop(hdev);
   614  err_cleanup_data:
   615          kfree(data);
   616  err_no_cleanup:
   617          hid_set_drvdata(hdev, NULL);
   618  
   619          return error;
   620  }

It's not clear to me what "hid_ll" means, also "no_cleanup" seems a bit
misleading, but the rest is pretty straight forward just by looking at
it.

regards,
dan carpenter