linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] cvt_fallthrough: A tool to convert /* fallthrough */ comments to fallthrough;
@ 2020-02-20 20:30 Joe Perches
  2020-02-21  0:21 ` Andrew Morton
  2020-03-07 21:30 ` Gustavo A. R. Silva
  0 siblings, 2 replies; 15+ messages in thread
From: Joe Perches @ 2020-02-20 20:30 UTC (permalink / raw)
  To: Andrew Morton, linux-kernel; +Cc: clang-built-linux

Convert /* fallthrough */ style comments to the pseudo-keyword fallthrough
to allow clang 10 and higher to work at finding missing fallthroughs too.

Requires a git repository and overwrites the input files.

Typical command use:
    ./scripts/cvt_fallthrough.pl <path|file>

i.e.:

   $ ./scripts/cvt_fallthrough.pl block
     converts all files in block and its subdirectories
   $ ./scripts/cvt_fallthrough.pl drivers/net/wireless/zydas/zd1201.c
     converts a single file

A fallthrough comment with additional comments is converted akin to:

-		/* fall through - maybe userspace knows this conn_id. */
+		fallthrough;    /* maybe userspace knows this conn_id */

A fallthrough comment or fallthrough; between successive case statements
is deleted.

e.g.:

    case FOO:
    	/* fallthrough */ (or fallthrough;)
    case BAR:

is converted to:

    case FOO:
    case BAR:

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/cvt_fallthrough.pl | 215 +++++++++++++++++++++++++++++++++++++
 1 file changed, 215 insertions(+)
 create mode 100755 scripts/cvt_fallthrough.pl

diff --git a/scripts/cvt_fallthrough.pl b/scripts/cvt_fallthrough.pl
new file mode 100755
index 000000..6585e4
--- /dev/null
+++ b/scripts/cvt_fallthrough.pl
@@ -0,0 +1,215 @@
+#!/usr/bin/perl -w
+
+# script to modify /* fallthrough */ style comments to fallthrough;
+# use: perl cvt_fallthrough.pl <paths|files>
+# e.g.: perl cvtfallthrough.pl drivers/net/ethernet/intel
+
+use strict;
+
+my $P = $0;
+my $modified = 0;
+my $quiet = 0;
+
+sub expand_tabs {
+    my ($str) = @_;
+
+    my $res = '';
+    my $n = 0;
+    for my $c (split(//, $str)) {
+	if ($c eq "\t") {
+	    $res .= ' ';
+	    $n++;
+	    for (; ($n % 8) != 0; $n++) {
+		$res .= ' ';
+	    }
+	    next;
+	}
+	$res .= $c;
+	$n++;
+    }
+
+    return $res;
+}
+
+my $args = join(" ", @ARGV);
+my $output = `git ls-files -- $args`;
+my @files = split("\n", $output);
+
+foreach my $file (@files) {
+    my $f;
+    my $cvt = 0;
+    my $del = 0;
+    my $text;
+
+# read the file
+
+    next if ((-d $file));
+
+    open($f, '<', $file)
+	or die "$P: Can't open $file for read\n";
+    $text = do { local($/) ; <$f> };
+    close($f);
+
+    next if ($text eq "");
+
+    # for style:
+
+    # /* <fallthrough comment> */
+    # case FOO:
+
+    # while (comment has fallthrough and next non-blank, non-continuation line is (case or default:)) {
+    #   remove newline, whitespace before, fallthrough comment and whitespace after
+    #   insert newline, adjusted spacing and fallthrough; newline
+    # }
+
+    my $count = 0;
+    my @fallthroughs = (
+	'fallthrough',
+	'@fallthrough@',
+	'lint -fallthrough[ \t]*',
+	'[ \t.!]*(?:ELSE,? |INTENTIONAL(?:LY)? )?',
+	'intentional(?:ly)?[ \t]*fall(?:(?:s | |-)[Tt]|t)hr(?:ough|u|ew)',
+	'(?:else,?\s*)?FALL(?:S | |-)?THR(?:OUGH|U|EW)[ \t.!]*(?:-[^\n\r]*)?',
+	'[ \t.!]*(?:Else,? |Intentional(?:ly)? )?',
+	'Fall(?:(?:s | |-)[Tt]|t)hr(?:ough|u|ew)[ \t.!]*(?:-[^\n\r]*)?',
+	'[ \t.!]*(?:[Ee]lse,? |[Ii]ntentional(?:ly)? )?',
+	'fall(?:s | |-)?thr(?:ough|u|ew)[ \t.!]*(?:-[^\n\r]*)?',
+    );
+    do {
+	$count = 0;
+	pos($text) = 0;
+	foreach my $ft (@fallthroughs) {
+	    my $regex = '(((?:[ \t]*\n)*[ \t]*)(/\*\s*(?!\*/)?\b' . "$ft" . '\s*(?!\*/)?\*/(?:[ \t]*\n)*)([ \t]*))(?:case\s+|default\s*:)';
+
+	    while ($text =~ m{${regex}}i) {
+		my $all_but_case = $1;
+		my $space_before = $2;
+		my $fallthrough = $3;
+		my $space_after = $4;
+		my $pos_before = $-[1];
+		my $pos_after = $+[3];
+
+		# try to maintain any additional comment on the same line
+		my $comment = "";
+		if ($regex =~ /\\r/) {
+		    $comment = $fallthrough;
+		    $comment =~ s@^/\*\s*@@;
+		    $comment =~ s@\s*\*/\s*$@@;
+		    $comment =~ s@^\s*(?:intentional(?:ly)?\s+|else,?\s*)?fall[s\-]*\s*thr(?:ough|u|ew)[\s\.\-]*@@i;
+		    $comment =~ s@\s+$@@;
+		    $comment =~ s@\.*$@@;
+		    $comment = "\t/* $comment */" if ($comment ne "");
+		}
+		substr($text, $pos_before, $pos_after - $pos_before, "");
+		substr($text, $pos_before, 0, "\n${space_after}\tfallthrough;${comment}\n");
+		pos($text) = $pos_before;
+		$count++;
+	    }
+	}
+	$cvt += $count;
+        } while ($count > 0);
+
+    # Reset the fallthroughs types to a single regex
+
+    @fallthroughs = (
+	'fall(?:(?:s | |-)[Tt]|t)hr(?:ough|u|ew)'
+    );
+
+    # Convert the // <fallthrough> style comments followed by case/default:
+
+    do {
+	$count = 0;
+	pos($text) = 0;
+	foreach my $ft (@fallthroughs) {
+	    my $regex = '(((?:[ \t]*\n)*[ \t]*)(//[ \t]*(?!\n)?\b' . "$ft" . '[ \t]*(?!\n)?\n(?:[ \t]*\n)*)([ \t]*))(?:case\s+|default\s*:)';
+	    while ($text =~ m{${regex}}i) {
+		my $all_but_case = $1;
+		my $space_before = $2;
+		my $fallthrough = $3;
+		my $space_after = $4;
+		my $pos_before = $-[1];
+		my $pos_after = $+[3];
+
+		substr($text, $pos_before, $pos_after - $pos_before, "");
+		substr($text, $pos_before, 0, "\n${space_after}\tfallthrough;\n");
+		pos($text) = $pos_before;
+		$count++;
+	    }
+	}
+	$cvt += $count;
+    } while ($count > 0);
+
+    # Convert /* fallthrough */ comment macro lines with trailing \
+
+    do {
+	$count = 0;
+	pos($text) = 0;
+	foreach my $ft (@fallthroughs) {
+	    my $regex = '((?:[ \t]*\\\\\n)*([ \t]*)(/\*[ \t]*\b' . "$ft" . '[ \t]*\*/)([ \t]*))\\\\\n*([ \t]*(?:case\s+|default\s*:))';
+
+	    while ($text =~ m{${regex}}i) {
+		my $all_but_case = $1;
+		my $space_before = $2;
+		my $fallthrough = $3;
+		my $space_after = $4;
+		my $pos_before = $-[2];
+		my $pos_after = $+[4];
+
+		my $oldline = "${space_before}${fallthrough}${space_after}";
+		my $len = length(expand_tabs($oldline));
+
+		my $newline = "${space_before}fallthrough;${space_after}";
+		$newline .= "\t" while (length(expand_tabs($newline)) < $len);
+
+		substr($text, $pos_before, $pos_after - $pos_before, "");
+		substr($text, $pos_before, 0, "$newline");
+		pos($text) = $pos_before;
+		$count++;
+	    }
+	}
+	$cvt += $count;
+    } while ($count > 0);
+
+    # delete any unnecessary fallthrough; between successive cases and default:
+
+    do {
+	pos($text) = 0;
+	$count = $text =~ s/(case\s+\w+\s*:\n)[ \t]*fallthrough;\n((?:\s*default\s*:|\s*case\s+\w+\s*:))/$1$2/;
+	$del += $count;
+    } while ($count > 0);
+
+# write the file if something was changed
+
+    if ($cvt > 0 || $del > 0) {
+	$modified = 1;
+
+	open($f, '>', $file)
+	    or die "$P: Can't open $file for write\n";
+	print $f $text;
+	close($f);
+
+	if (!$quiet) {
+	    print "fallthroughs:";
+	    print "	converted: $cvt" if ($cvt > 0);
+	    print "	deleted: $del" if ($del > 0);
+	    print "	$file\n";
+	}
+    }
+}
+
+if ($modified && !$quiet) {
+    print <<EOT;
+
+Warning: these changes may not be correct.
+
+These changes should be carefully reviewed manually and not combined with
+any functional changes.
+
+Compile, build and test your changes.
+
+You should understand and be responsible for all object changes.
+
+Make sure you read Documentation/SubmittingPatches before sending
+any changes to reviewers, maintainers or mailing lists.
+EOT
+}
-- 
2.24.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] cvt_fallthrough: A tool to convert /* fallthrough */ comments to fallthrough;
  2020-02-20 20:30 [PATCH] cvt_fallthrough: A tool to convert /* fallthrough */ comments to fallthrough; Joe Perches
@ 2020-02-21  0:21 ` Andrew Morton
  2020-02-21  2:24   ` Joe Perches
  2020-03-09 19:36   ` Nick Desaulniers
  2020-03-07 21:30 ` Gustavo A. R. Silva
  1 sibling, 2 replies; 15+ messages in thread
From: Andrew Morton @ 2020-02-21  0:21 UTC (permalink / raw)
  To: Joe Perches; +Cc: linux-kernel, clang-built-linux

On Thu, 20 Feb 2020 12:30:21 -0800 Joe Perches <joe@perches.com> wrote:

> Convert /* fallthrough */ style comments to the pseudo-keyword fallthrough
> to allow clang 10 and higher to work at finding missing fallthroughs too.
> 
> Requires a git repository and overwrites the input files.
> 
> Typical command use:
>     ./scripts/cvt_fallthrough.pl <path|file>
> 
> i.e.:
> 
>    $ ./scripts/cvt_fallthrough.pl block
>      converts all files in block and its subdirectories
>    $ ./scripts/cvt_fallthrough.pl drivers/net/wireless/zydas/zd1201.c
>      converts a single file
> 
> A fallthrough comment with additional comments is converted akin to:
> 
> -		/* fall through - maybe userspace knows this conn_id. */
> +		fallthrough;    /* maybe userspace knows this conn_id */
> 
> A fallthrough comment or fallthrough; between successive case statements
> is deleted.
> 
> e.g.:
> 
>     case FOO:
>     	/* fallthrough */ (or fallthrough;)
>     case BAR:
> 
> is converted to:
> 
>     case FOO:
>     case BAR:
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  scripts/cvt_fallthrough.pl | 215 +++++++++++++++++++++++++++++++++++++

Do we need this in the tree long-term?  Or is it a matters of "hey
Linus, please run this" then something like add a checkpatch rule to
catch future slipups?


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] cvt_fallthrough: A tool to convert /* fallthrough */ comments to fallthrough;
  2020-02-21  0:21 ` Andrew Morton
@ 2020-02-21  2:24   ` Joe Perches
  2020-03-09 19:36   ` Nick Desaulniers
  1 sibling, 0 replies; 15+ messages in thread
From: Joe Perches @ 2020-02-21  2:24 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel, clang-built-linux, Gustavo A. R. Silva

On Thu, 2020-02-20 at 16:21 -0800, Andrew Morton wrote:
> On Thu, 20 Feb 2020 12:30:21 -0800 Joe Perches <joe@perches.com> wrote:
> 
> > Convert /* fallthrough */ style comments to the pseudo-keyword fallthrough
> > to allow clang 10 and higher to work at finding missing fallthroughs too.
> > 
> > Requires a git repository and overwrites the input files.
> > 
> > Typical command use:
> >     ./scripts/cvt_fallthrough.pl <path|file>
> > 
> > i.e.:
> > 
> >    $ ./scripts/cvt_fallthrough.pl block
> >      converts all files in block and its subdirectories
> >    $ ./scripts/cvt_fallthrough.pl drivers/net/wireless/zydas/zd1201.c
> >      converts a single file
> > 
> > A fallthrough comment with additional comments is converted akin to:
> > 
> > -		/* fall through - maybe userspace knows this conn_id. */
> > +		fallthrough;    /* maybe userspace knows this conn_id */
> > 
> > A fallthrough comment or fallthrough; between successive case statements
> > is deleted.
> > 
> > e.g.:
> > 
> >     case FOO:
> >     	/* fallthrough */ (or fallthrough;)
> >     case BAR:
> > 
> > is converted to:
> > 
> >     case FOO:
> >     case BAR:
> > 
> > Signed-off-by: Joe Perches <joe@perches.com>
> > ---
> >  scripts/cvt_fallthrough.pl | 215 +++++++++++++++++++++++++++++++++++++
> 
> Do we need this in the tree long-term?

Out-of-tree scripts aren't used by trivial patch submitters.

So no, not really.  I think it's a 'good to have, short term'
script useful until at least most all of the conversions occur.

And I think having multiple concurrent styles for fallthrough
isn't great.

And I don't have the patience of someone like Gustavo Silva to
painstakin
gly shepherd hundreds of little patches either.
(thanks Gustavo, good
work...)

And it would be nice though to have some mechanism to get
scripted patches applied, either by subsystem or treewide.

> Or is it a matters of "hey
> Linus, please run this" then something like add a checkpatch rule to
> catch future slipups?

The checkpatch rule was added a week ago.
https://lore.kernel.org/lkml/8b6c1b9031ab9f3cdebada06b8d46467f1492d68.camel@perches.com/

Adding a --fix option wouldn't work as well as this script
to do conversions as the script is moderately complicated.

It does seem a treewide conversion like this could have a
small impact on stable trees, so any conversion should
probably be done by subsystem.

Anyway, the script does a pretty reasonable job at
conversions of the various styles of fallthrough comments.

Though there are some conversion that are not done when a
/* fallthrough */ comment is followed by another comment
before another case like:

	case FOO:
		/* fall through */
		/* another comment */
	case BAR:

Anyway, using today's -next, a treewide diffstat is:

$ git diff --shortstat
 1861 files changed, 4113 insertions(+), 4762 deletions(-)

And these are the most common conversions:

   2278 /* fall through */
    441 /* Fall through */
    253 /* fallthrough */
    164 /* FALLTHROUGH */
    116 /* fall-through */
     84 /* Fallthrough */
     33 /* FALL THROUGH */
     31 /* Fall through */				\
     27 /* FALLTHRU */
     24 /*FALLTHRU*/
     24 /* fallthru */
     19 /* fall thru */
     19 /* Fall Through */
     19 /* Fall-through */
     16 /* Else, fall through */
     15 /* fall-thru */
     13 /* Intentional fallthrough */
     13 /*FALLTHROUGH*/
     12 /* Fall thru */
     12 /* else, fall through */
     10 /*lint -fallthrough */
      9 /* Fall through. */
      9 }	/* fallthrough */
      8 /*fall through*/



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] cvt_fallthrough: A tool to convert /* fallthrough */ comments to fallthrough;
  2020-02-20 20:30 [PATCH] cvt_fallthrough: A tool to convert /* fallthrough */ comments to fallthrough; Joe Perches
  2020-02-21  0:21 ` Andrew Morton
@ 2020-03-07 21:30 ` Gustavo A. R. Silva
  2020-03-08  3:01   ` Joe Perches
  1 sibling, 1 reply; 15+ messages in thread
From: Gustavo A. R. Silva @ 2020-03-07 21:30 UTC (permalink / raw)
  To: Joe Perches, Andrew Morton, linux-kernel; +Cc: clang-built-linux



On 2/20/20 14:30, Joe Perches wrote:
> Convert /* fallthrough */ style comments to the pseudo-keyword fallthrough
> to allow clang 10 and higher to work at finding missing fallthroughs too.
> 
> Requires a git repository and overwrites the input files.
> 
> Typical command use:
>     ./scripts/cvt_fallthrough.pl <path|file>
> 
> i.e.:
> 
>    $ ./scripts/cvt_fallthrough.pl block
>      converts all files in block and its subdirectories
>    $ ./scripts/cvt_fallthrough.pl drivers/net/wireless/zydas/zd1201.c
>      converts a single file
> 
> A fallthrough comment with additional comments is converted akin to:
> 
> -		/* fall through - maybe userspace knows this conn_id. */
> +		fallthrough;    /* maybe userspace knows this conn_id */
> 
> A fallthrough comment or fallthrough; between successive case statements
> is deleted.
> 
> e.g.:
> 
>     case FOO:
>     	/* fallthrough */ (or fallthrough;)
>     case BAR:
> 
> is converted to:
> 
>     case FOO:
>     case BAR:
> 

I think the script should only replace the comments and refrain from removing any blank
lines:

--- a/drivers/usb/musb/musb_gadget_ep0.c
+++ b/drivers/usb/musb/musb_gadget_ep0.c
@@ -739,8 +739,7 @@ irqreturn_t musb_g_ep0_irq(struct musb *musb)
                        musb_writeb(mbase, MUSB_TESTMODE,
                                        musb->test_mode_nr);
                }
-               /* FALLTHROUGH */
-
+               fallthrough;
        case MUSB_EP0_STAGE_STATUSOUT:
                /* end of sequence #1: write to host (TX state) */
                {
@@ -771,8 +770,7 @@ irqreturn_t musb_g_ep0_irq(struct musb *musb)
                 */
                retval = IRQ_HANDLED;
                musb->ep0_state = MUSB_EP0_STAGE_SETUP;
-               /* FALLTHROUGH */
-
+               fallthrough;
        case MUSB_EP0_STAGE_SETUP:
 setup:
                if (csr & MUSB_CSR0_RXPKTRDY) {

Some people consistently add blank lines as part of their code style, and if I were
one of those people, I wouldn't like to have such lines removed.

--
Gustavo


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] cvt_fallthrough: A tool to convert /* fallthrough */ comments to fallthrough;
  2020-03-07 21:30 ` Gustavo A. R. Silva
@ 2020-03-08  3:01   ` Joe Perches
  2020-03-08  6:46     ` Gustavo A. R. Silva
  0 siblings, 1 reply; 15+ messages in thread
From: Joe Perches @ 2020-03-08  3:01 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Andrew Morton, linux-kernel; +Cc: clang-built-linux

On Sat, 2020-03-07 at 15:30 -0600, Gustavo A. R. Silva wrote:
> Some people consistently add blank lines as part of their code style,
> and if I were
> one of those people, I wouldn't like to have such lines removed.

It's a patch generator, it's not perfect.
Nothing is nor ever will be.
It's quite simple to add blank lines if that's
what any maintainer desires.



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] cvt_fallthrough: A tool to convert /* fallthrough */ comments to fallthrough;
  2020-03-08  3:01   ` Joe Perches
@ 2020-03-08  6:46     ` Gustavo A. R. Silva
  2020-03-08  7:02       ` Joe Perches
  0 siblings, 1 reply; 15+ messages in thread
From: Gustavo A. R. Silva @ 2020-03-08  6:46 UTC (permalink / raw)
  To: Joe Perches, Andrew Morton, linux-kernel; +Cc: clang-built-linux



On 3/7/20 21:01, Joe Perches wrote:
> On Sat, 2020-03-07 at 15:30 -0600, Gustavo A. R. Silva wrote:
>> Some people consistently add blank lines as part of their code style,
>> and if I were
>> one of those people, I wouldn't like to have such lines removed.
> 
> It's a patch generator, it's not perfect.
> Nothing is nor ever will be.

Wise words. The thing is that this is feedback over a proposed
patch.

> It's quite simple to add blank lines if that's
> what any maintainer desires.
> 

I'm not sure if you are saying that it's not a problem to
update your proposed patch, or if you are suggesting that
the maintainers will have the predisposition of applying
patches that will modify their coding style and then go and
willingly fix that. I doubt the latter, though.

--
Gustavo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] cvt_fallthrough: A tool to convert /* fallthrough */ comments to fallthrough;
  2020-03-08  6:46     ` Gustavo A. R. Silva
@ 2020-03-08  7:02       ` Joe Perches
  2020-03-08  7:11         ` Gustavo A. R. Silva
  0 siblings, 1 reply; 15+ messages in thread
From: Joe Perches @ 2020-03-08  7:02 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Andrew Morton, linux-kernel; +Cc: clang-built-linux

On Sun, 2020-03-08 at 00:46 -0600, Gustavo A. R. Silva wrote:
> 
> On 3/7/20 21:01, Joe Perches wrote:
> > On Sat, 2020-03-07 at 15:30 -0600, Gustavo A. R. Silva wrote:
> > > Some people consistently add blank lines as part of their code style,
> > > and if I were
> > > one of those people, I wouldn't like to have such lines removed.
> > 
> > It's a patch generator, it's not perfect.
> > Nothing is nor ever will be.
> 
> Wise words. The thing is that this is feedback over a proposed
> patch.
> 
> > It's quite simple to add blank lines if that's
> > what any maintainer desires.
> > 
> 
> I'm not sure if you are saying that it's not a problem to
> update your proposed patch,

It's not a problem with my proposed patch.
Consistency is good.

Nearly all uses in mm do not have blank lines
after break.

> or if you are suggesting that
> the maintainers will have the predisposition of applying
> patches that will modify their coding style and then go and
> willingly fix that. I doubt the latter, though.

If any do actually use the script, I guess we'll see.

> --
> Gustavo


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] cvt_fallthrough: A tool to convert /* fallthrough */ comments to fallthrough;
  2020-03-08  7:02       ` Joe Perches
@ 2020-03-08  7:11         ` Gustavo A. R. Silva
  2020-03-08  8:58           ` Joe Perches
  0 siblings, 1 reply; 15+ messages in thread
From: Gustavo A. R. Silva @ 2020-03-08  7:11 UTC (permalink / raw)
  To: Joe Perches, Andrew Morton, linux-kernel; +Cc: clang-built-linux



On 3/8/20 01:02, Joe Perches wrote:

>> or if you are suggesting that
>> the maintainers will have the predisposition of applying
>> patches that will modify their coding style and then go and
>> willingly fix that. I doubt the latter, though.
> 
> If any do actually use the script, I guess we'll see.
> 

Yep. In the meantime is a NACK from me for this version
of your patch.

Thanks
--
Gustavo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] cvt_fallthrough: A tool to convert /* fallthrough */ comments to fallthrough;
  2020-03-08  7:11         ` Gustavo A. R. Silva
@ 2020-03-08  8:58           ` Joe Perches
  2020-03-08 19:14             ` Gustavo A. R. Silva
  0 siblings, 1 reply; 15+ messages in thread
From: Joe Perches @ 2020-03-08  8:58 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Andrew Morton, linux-kernel; +Cc: clang-built-linux

On Sun, 2020-03-08 at 01:11 -0600, Gustavo A. R. Silva wrote:
> On 3/8/20 01:02, Joe Perches wrote:
> > > or if you are suggesting that
> > > the maintainers will have the predisposition of applying
> > > patches that will modify their coding style and then go and
> > > willingly fix that. I doubt the latter, though.
> > 
> > If any do actually use the script, I guess we'll see.
> > 
> Yep. In the meantime is a NACK from me for this version
> of your patch.

Generic code reformatters of comments to code are not
particularly common.

Instead of suggesting a nak for something that works
pretty well, it would perhaps be more fruitful if you
were to try either to write something else or try to
improve the existing code.

I'd be interested in seeing whatever code you produce.

cheers, Joe



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] cvt_fallthrough: A tool to convert /* fallthrough */ comments to fallthrough;
  2020-03-08  8:58           ` Joe Perches
@ 2020-03-08 19:14             ` Gustavo A. R. Silva
  2020-03-08 19:44               ` Joe Perches
  0 siblings, 1 reply; 15+ messages in thread
From: Gustavo A. R. Silva @ 2020-03-08 19:14 UTC (permalink / raw)
  To: Joe Perches, Andrew Morton, linux-kernel; +Cc: clang-built-linux



On 3/8/20 03:58, Joe Perches wrote:
> On Sun, 2020-03-08 at 01:11 -0600, Gustavo A. R. Silva wrote:
>> On 3/8/20 01:02, Joe Perches wrote:
>>>> or if you are suggesting that
>>>> the maintainers will have the predisposition of applying
>>>> patches that will modify their coding style and then go and
>>>> willingly fix that. I doubt the latter, though.
>>>
>>> If any do actually use the script, I guess we'll see.
>>>
>> Yep. In the meantime is a NACK from me for this version
>> of your patch.
> 
> Generic code reformatters of comments to code are not
> particularly common.
> 

I might not be getting my point across. It's no a matter of
reformatting something. It's the opposite, it's a matter of
not messing (removing existing blank lines) with the current
format and merely focusing on replacing comments.

--
Gustavo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] cvt_fallthrough: A tool to convert /* fallthrough */ comments to fallthrough;
  2020-03-08 19:14             ` Gustavo A. R. Silva
@ 2020-03-08 19:44               ` Joe Perches
  2020-03-08 22:04                 ` Gustavo A. R. Silva
  0 siblings, 1 reply; 15+ messages in thread
From: Joe Perches @ 2020-03-08 19:44 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Andrew Morton, linux-kernel; +Cc: clang-built-linux

On Sun, 2020-03-08 at 14:14 -0500, Gustavo A. R. Silva wrote:
> 
> On 3/8/20 03:58, Joe Perches wrote:
> > On Sun, 2020-03-08 at 01:11 -0600, Gustavo A. R. Silva wrote:
> > > On 3/8/20 01:02, Joe Perches wrote:
> > > > > or if you are suggesting that
> > > > > the maintainers will have the predisposition of applying
> > > > > patches that will modify their coding style and then go and
> > > > > willingly fix that. I doubt the latter, though.
> > > > 
> > > > If any do actually use the script, I guess we'll see.
> > > > 
> > > Yep. In the meantime is a NACK from me for this version
> > > of your patch.
> > 
> > Generic code reformatters of comments to code are not
> > particularly common.
> > 
> 
> It's no a matter of
> reformatting something. It's the opposite, it's a matter of
> not messing (removing existing blank lines) with the current
> format and merely focusing on replacing comments.

You are not correct in your assumption.

This is precisely reformatting of comments to code.

Nor are you correct in what appears to be your
general point.  It's quite fine to reformat comments
for consistency.



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] cvt_fallthrough: A tool to convert /* fallthrough */ comments to fallthrough;
  2020-03-08 19:44               ` Joe Perches
@ 2020-03-08 22:04                 ` Gustavo A. R. Silva
  2020-03-08 22:05                   ` Joe Perches
  0 siblings, 1 reply; 15+ messages in thread
From: Gustavo A. R. Silva @ 2020-03-08 22:04 UTC (permalink / raw)
  To: Joe Perches, Andrew Morton, linux-kernel; +Cc: clang-built-linux



The point is: will you update your patch so it doesn't remove
existing blank lines from the code?

If yes, send v2, please.

Thanks
--
Gustavo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] cvt_fallthrough: A tool to convert /* fallthrough */ comments to fallthrough;
  2020-03-08 22:04                 ` Gustavo A. R. Silva
@ 2020-03-08 22:05                   ` Joe Perches
  0 siblings, 0 replies; 15+ messages in thread
From: Joe Perches @ 2020-03-08 22:05 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Andrew Morton, linux-kernel; +Cc: clang-built-linux

On Sun, 2020-03-08 at 17:04 -0500, Gustavo A. R. Silva wrote:
> 
> The point is: will you update your patch so it doesn't remove
> existing blank lines from the code?

No.

In the first place, I believe what I did is
the preferred style

You need to look at the code and see what you
think needs to be done and then do it instead
of merely sending a fairly pointless nak when
in fact you can't really nak it.  You could
reasonably object to it, but you aren't really
a listed maintainer of any subsystem and you
don't have an upstream tree that is being pulled
by anyone.

I believe it would not trivial to implement
your desire.  You are welcome to try.



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] cvt_fallthrough: A tool to convert /* fallthrough */ comments to fallthrough;
  2020-02-21  0:21 ` Andrew Morton
  2020-02-21  2:24   ` Joe Perches
@ 2020-03-09 19:36   ` Nick Desaulniers
  2020-03-09 19:55     ` Joe Perches
  1 sibling, 1 reply; 15+ messages in thread
From: Nick Desaulniers @ 2020-03-09 19:36 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Joe Perches, LKML, clang-built-linux

On Thu, Feb 20, 2020 at 4:21 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Thu, 20 Feb 2020 12:30:21 -0800 Joe Perches <joe@perches.com> wrote:
>
> > Convert /* fallthrough */ style comments to the pseudo-keyword fallthrough
> > to allow clang 10 and higher to work at finding missing fallthroughs too.
> >
> > Requires a git repository and overwrites the input files.
> >
> > Typical command use:
> >     ./scripts/cvt_fallthrough.pl <path|file>
> >
> > i.e.:
> >
> >    $ ./scripts/cvt_fallthrough.pl block
> >      converts all files in block and its subdirectories
> >    $ ./scripts/cvt_fallthrough.pl drivers/net/wireless/zydas/zd1201.c
> >      converts a single file
> >
> > A fallthrough comment with additional comments is converted akin to:
> >
> > -             /* fall through - maybe userspace knows this conn_id. */
> > +             fallthrough;    /* maybe userspace knows this conn_id */
> >
> > A fallthrough comment or fallthrough; between successive case statements
> > is deleted.
> >
> > e.g.:
> >
> >     case FOO:
> >       /* fallthrough */ (or fallthrough;)
> >     case BAR:
> >
> > is converted to:
> >
> >     case FOO:
> >     case BAR:
> >
> > Signed-off-by: Joe Perches <joe@perches.com>
> > ---
> >  scripts/cvt_fallthrough.pl | 215 +++++++++++++++++++++++++++++++++++++
>
> Do we need this in the tree long-term?  Or is it a matters of "hey
> Linus, please run this" then something like add a checkpatch rule to
> catch future slipups?

Just for some added context, please see
https://reviews.llvm.org/D73852, where support for parsing some forms
of fallthrough statements was added to Clang in a broken state by a
contributor, but then ripped out by the code owner (of the clang front
end to LLVM, and also happens to be the C++ ISO spec editor).  He
provides further clarification
https://bugs.llvm.org/show_bug.cgi?id=43465#c37.

I'm inclined to agree with him; to implement this, we need to keep
around comments for semantic analyses, a later phase of compilation
than preprocessing.  It feels like a layering violation to either not
discard comments as soon as possible, or emit diagnostics from the
preprocessor.  And as Joe's data shows, there's the classic issue
faced when using regexes to solve a problem; suddenly you now have two
problems.
https://xkcd.com/1171/

I would like to see this patch landed, though I am curious as toward's
Andrew's question ('Or is it a matters of "hey Linus, please run
this"') of what's the imagined workflow here, since it seems like the
script needs to be run per file. I suppose you could still do that
treewide, but is that the intention, or is it to do so on a per
subsystem level?



--
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] cvt_fallthrough: A tool to convert /* fallthrough */ comments to fallthrough;
  2020-03-09 19:36   ` Nick Desaulniers
@ 2020-03-09 19:55     ` Joe Perches
  0 siblings, 0 replies; 15+ messages in thread
From: Joe Perches @ 2020-03-09 19:55 UTC (permalink / raw)
  To: Nick Desaulniers, Andrew Morton; +Cc: LKML, clang-built-linux

On Mon, 2020-03-09 at 12:36 -0700, Nick Desaulniers wrote:
> On Thu, Feb 20, 2020 at 4:21 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Thu, 20 Feb 2020 12:30:21 -0800 Joe Perches <joe@perches.com> wrote:
> > 
> > > Convert /* fallthrough */ style comments to the pseudo-keyword fallthrough
> > > to allow clang 10 and higher to work at finding missing fallthroughs too.
> > > 
> > > Requires a git repository and overwrites the input files.
> > > 
> > > Typical command use:
> > >     ./scripts/cvt_fallthrough.pl <path|file>
> > > 
> > > i.e.:
> > > 
> > >    $ ./scripts/cvt_fallthrough.pl block
> > >      converts all files in block and its subdirectories
> > >    $ ./scripts/cvt_fallthrough.pl drivers/net/wireless/zydas/zd1201.c
> > >      converts a single file
> > > 
> > > A fallthrough comment with additional comments is converted akin to:
> > > 
> > > -             /* fall through - maybe userspace knows this conn_id. */
> > > +             fallthrough;    /* maybe userspace knows this conn_id */
> > > 
> > > A fallthrough comment or fallthrough; between successive case statements
> > > is deleted.
> > > 
> > > e.g.:
> > > 
> > >     case FOO:
> > >       /* fallthrough */ (or fallthrough;)
> > >     case BAR:
> > > 
> > > is converted to:
> > > 
> > >     case FOO:
> > >     case BAR:
> > > 
> > > Signed-off-by: Joe Perches <joe@perches.com>
> > > ---
> > >  scripts/cvt_fallthrough.pl | 215 +++++++++++++++++++++++++++++++++++++
> > 
> > Do we need this in the tree long-term?  Or is it a matters of "hey
> > Linus, please run this" then something like add a checkpatch rule to
> > catch future slipups?
> 
> Just for some added context, please see
> https://reviews.llvm.org/D73852, where support for parsing some forms
> of fallthrough statements was added to Clang in a broken state by a
> contributor, but then ripped out by the code owner (of the clang front
> end to LLVM, and also happens to be the C++ ISO spec editor).  He
> provides further clarification
> https://bugs.llvm.org/show_bug.cgi?id=43465#c37.
> 
> I'm inclined to agree with him; to implement this, we need to keep
> around comments for semantic analyses, a later phase of compilation
> than preprocessing.  It feels like a layering violation to either not
> discard comments as soon as possible, or emit diagnostics from the
> preprocessor.  And as Joe's data shows, there's the classic issue
> faced when using regexes to solve a problem; suddenly you now have two
> problems.
> https://xkcd.com/1171/
> 
> I would like to see this patch landed, though I am curious as toward's
> Andrew's question ('Or is it a matters of "hey Linus, please run
> this"') of what's the imagined workflow here, since it seems like the
> script needs to be run per file. I suppose you could still do that
> treewide, but is that the intention, or is it to do so on a per
> subsystem level?

A single treewide run of a script like this really could make
it quite hard to later backport various fixes to stable trees.

A depth-first per-maintained subsystem run of the script with
commits could be useful and would much more easily allow backports.

Unfortunately there's no tool to apply such a script to the tree
per subsystem as far as I know.

Such a depth-first apply and commit tool could really be quite
useful though.



^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2020-03-09 19:56 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-20 20:30 [PATCH] cvt_fallthrough: A tool to convert /* fallthrough */ comments to fallthrough; Joe Perches
2020-02-21  0:21 ` Andrew Morton
2020-02-21  2:24   ` Joe Perches
2020-03-09 19:36   ` Nick Desaulniers
2020-03-09 19:55     ` Joe Perches
2020-03-07 21:30 ` Gustavo A. R. Silva
2020-03-08  3:01   ` Joe Perches
2020-03-08  6:46     ` Gustavo A. R. Silva
2020-03-08  7:02       ` Joe Perches
2020-03-08  7:11         ` Gustavo A. R. Silva
2020-03-08  8:58           ` Joe Perches
2020-03-08 19:14             ` Gustavo A. R. Silva
2020-03-08 19:44               ` Joe Perches
2020-03-08 22:04                 ` Gustavo A. R. Silva
2020-03-08 22:05                   ` Joe Perches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).