[PATCH] checkpatch: add --fix option for OPEN_BRACE issues

[PATCH] checkpatch: add --fix option for OPEN_BRACE issues

[Dwaipayan Ray]

Brace style misuses of the following types are now
corrected:

int foo(int bar,
        int baz) { bar++;
    return bar + baz;
}

int foo(int bar,
        int baz) {
    return bar + baz;
}

if (bar &&
    baz)
{   bar++;
    baz++;
}

if (bar &&
    baz)
{
    bar++;
    baz++;
}

Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
---
 scripts/checkpatch.pl | 38 +++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 0da6422cd0fd..8da6cde20c68 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3937,9 +3937,23 @@ sub process {
 			#print "pre<$pre_ctx>\nline<$line>\nctx<$ctx>\nnext<$lines[$ctx_ln - 1]>\n";
 
 			if ($ctx !~ /{\s*/ && defined($lines[$ctx_ln - 1]) && $lines[$ctx_ln - 1] =~ /^\+\s*{/) {
-				ERROR("OPEN_BRACE",
-				      "that open brace { should be on the previous line\n" .
-					"$here\n$ctx\n$rawlines[$ctx_ln - 1]\n");
+				if (ERROR("OPEN_BRACE",
+					  "that open brace { should be on the previous line\n" .
+						"$here\n$ctx\n$rawlines[$ctx_ln - 1]\n") &&
+				    $fix) {
+					my $line1 = $rawlines[$ctx_ln - 2];
+					my $line2 = $rawlines[$ctx_ln - 1];
+					fix_delete_line($ctx_ln - 2, $line1);
+					fix_delete_line($ctx_ln - 1, $line2);
+
+					my $fixedline = rtrim($line1) . " {";
+					fix_insert_line($ctx_ln - 1, $fixedline);
+					$fixedline = $line2;
+					$fixedline =~ s/^(.\s*)\{\s*/$1\t/;
+					if ($fixedline !~ /^\+\s*$/) {
+						fix_insert_line($ctx_ln - 1, $fixedline);
+					}
+				}
 			}
 			if ($level == 0 && $pre_ctx !~ /}\s*while\s*\($/ &&
 			    $ctx =~ /\)\s*\;\s*$/ &&
@@ -6659,16 +6673,30 @@ sub process {
 			my $ok = 0;
 			my $cnt = statement_rawlines($stat);
 			my $herectx = $here . "\n";
+			my $cur_ln = $linenr - 1;
 			for (my $n = 0; $n < $cnt; $n++) {
 				my $rl = raw_line($linenr, $n);
 				$herectx .=  $rl . "\n";
 				$ok = 1 if ($rl =~ /^[ \+]\{/);
 				$ok = 1 if ($rl =~ /\{/ && $n == 0);
 				last if $rl =~ /^[ \+].*\{/;
+				$cur_ln++;
 			}
 			if (!$ok) {
-				ERROR("OPEN_BRACE",
-				      "open brace '{' following function definitions go on the next line\n" . $herectx);
+				if (ERROR("OPEN_BRACE",
+					  "open brace '{' following function definitions go on the next line\n" . $herectx) &&
+				    $fix && $rawlines[$cur_ln] =~ /^\+/) {
+					fix_delete_line($cur_ln, $rawlines[$cur_ln]);
+					my $fixed_line = $rawlines[$cur_ln];
+					$fixed_line =~ /(^.*\)\s*)\{(.*)$/;
+					my $line1 = rtrim($1);
+					my $line2 = $2;
+					fix_insert_line($cur_ln, $line1);
+					fix_insert_line($cur_ln, "\+{");
+					if ($line2 !~ /^\s*$/) {
+						fix_insert_line($cur_ln, "\+\t" . trim($line2));
+					}
+				}
 			}
 		}
 
-- 
2.27.0

Re: [PATCH] checkpatch: add --fix option for OPEN_BRACE issues

[Joe Perches]

On Wed, 2020-11-18 at 18:10 +0530, Dwaipayan Ray wrote:
> Brace style misuses of the following types are now
> corrected:
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -3937,9 +3937,23 @@ sub process {
>  			#print "pre<$pre_ctx>\nline<$line>\nctx<$ctx>\nnext<$lines[$ctx_ln - 1]>\n";
>  
> 
>  			if ($ctx !~ /{\s*/ && defined($lines[$ctx_ln - 1]) && $lines[$ctx_ln - 1] =~ /^\+\s*{/) {
> -				ERROR("OPEN_BRACE",
> -				      "that open brace { should be on the previous line\n" .
> -					"$here\n$ctx\n$rawlines[$ctx_ln - 1]\n");
> +				if (ERROR("OPEN_BRACE",
> +					  "that open brace { should be on the previous line\n" .
> +						"$here\n$ctx\n$rawlines[$ctx_ln - 1]\n") &&
> +				    $fix) {
> +					my $line1 = $rawlines[$ctx_ln - 2];

How are you sure that in a patch context this line always starts with /^\+/ ?

> +					my $line2 = $rawlines[$ctx_ln - 1];
> +					fix_delete_line($ctx_ln - 2, $line1);
> +					fix_delete_line($ctx_ln - 1, $line2);
> +
> +					my $fixedline = rtrim($line1) . " {";
> +					fix_insert_line($ctx_ln - 1, $fixedline);
> +					$fixedline = $line2;
> +					$fixedline =~ s/^(.\s*)\{\s*/$1\t/;
> +					if ($fixedline !~ /^\+\s*$/) {
> +						fix_insert_line($ctx_ln - 1, $fixedline);
> +					}
> +				}

Re: [PATCH] checkpatch: add --fix option for OPEN_BRACE issues

[Dwaipayan Ray]

On Wed, Nov 18, 2020 at 11:44 PM Joe Perches <joe@perches.com> wrote:
>
> On Wed, 2020-11-18 at 18:10 +0530, Dwaipayan Ray wrote:
> > Brace style misuses of the following types are now
> > corrected:
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -3937,9 +3937,23 @@ sub process {
> >                       #print "pre<$pre_ctx>\nline<$line>\nctx<$ctx>\nnext<$lines[$ctx_ln - 1]>\n";
> >
> >
> >                       if ($ctx !~ /{\s*/ && defined($lines[$ctx_ln - 1]) && $lines[$ctx_ln - 1] =~ /^\+\s*{/) {
> > -                             ERROR("OPEN_BRACE",
> > -                                   "that open brace { should be on the previous line\n" .
> > -                                     "$here\n$ctx\n$rawlines[$ctx_ln - 1]\n");
> > +                             if (ERROR("OPEN_BRACE",
> > +                                       "that open brace { should be on the previous line\n" .
> > +                                             "$here\n$ctx\n$rawlines[$ctx_ln - 1]\n") &&
> > +                                 $fix) {
> > +                                     my $line1 = $rawlines[$ctx_ln - 2];
>
> How are you sure that in a patch context this line always starts with /^\+/ ?

Hi,
I followed it from the other fixes for OPEN_BRACE which were already
there. In the patch context if the lines are added then only I think the fix
should be triggered. Other instances should not be modified.

Thanks,
Dwaipayan.

Re: [PATCH] checkpatch: add --fix option for OPEN_BRACE issues

[Joe Perches]

On Thu, 2020-11-19 at 00:03 +0530, Dwaipayan Ray wrote:
> On Wed, Nov 18, 2020 at 11:44 PM Joe Perches <joe@perches.com> wrote:
> > 
> > On Wed, 2020-11-18 at 18:10 +0530, Dwaipayan Ray wrote:
> > > Brace style misuses of the following types are now
> > > corrected:
> > []
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > []
> > > @@ -3937,9 +3937,23 @@ sub process {
> > >                       #print "pre<$pre_ctx>\nline<$line>\nctx<$ctx>\nnext<$lines[$ctx_ln - 1]>\n";
> > > 
> > > 
> > >                       if ($ctx !~ /{\s*/ && defined($lines[$ctx_ln - 1]) && $lines[$ctx_ln - 1] =~ /^\+\s*{/) {
> > > -                             ERROR("OPEN_BRACE",
> > > -                                   "that open brace { should be on the previous line\n" .
> > > -                                     "$here\n$ctx\n$rawlines[$ctx_ln - 1]\n");
> > > +                             if (ERROR("OPEN_BRACE",
> > > +                                       "that open brace { should be on the previous line\n" .
> > > +                                             "$here\n$ctx\n$rawlines[$ctx_ln - 1]\n") &&
> > > +                                 $fix) {
> > > +                                     my $line1 = $rawlines[$ctx_ln - 2];
> > 
> > How are you sure that in a patch context this line always starts with /^\+/ ?
> 
> Hi,
> I followed it from the other fixes for OPEN_BRACE which were already
> there. In the patch context if the lines are added then only I think the fix
> should be triggered. Other instances should not be modified.

As far as I know there are no existing uses of --fix with OPEN_BRACE.

Re: [PATCH] checkpatch: add --fix option for OPEN_BRACE issues

[Dwaipayan Ray]

On Thu, Nov 19, 2020 at 12:09 AM Joe Perches <joe@perches.com> wrote:
>
> On Thu, 2020-11-19 at 00:03 +0530, Dwaipayan Ray wrote:
> > On Wed, Nov 18, 2020 at 11:44 PM Joe Perches <joe@perches.com> wrote:
> > >
> > > On Wed, 2020-11-18 at 18:10 +0530, Dwaipayan Ray wrote:
> > > > Brace style misuses of the following types are now
> > > > corrected:
> > > []
> > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > []
> > > > @@ -3937,9 +3937,23 @@ sub process {
> > > >                       #print "pre<$pre_ctx>\nline<$line>\nctx<$ctx>\nnext<$lines[$ctx_ln - 1]>\n";
> > > >
> > > >
> > > >                       if ($ctx !~ /{\s*/ && defined($lines[$ctx_ln - 1]) && $lines[$ctx_ln - 1] =~ /^\+\s*{/) {
> > > > -                             ERROR("OPEN_BRACE",
> > > > -                                   "that open brace { should be on the previous line\n" .
> > > > -                                     "$here\n$ctx\n$rawlines[$ctx_ln - 1]\n");
> > > > +                             if (ERROR("OPEN_BRACE",
> > > > +                                       "that open brace { should be on the previous line\n" .
> > > > +                                             "$here\n$ctx\n$rawlines[$ctx_ln - 1]\n") &&
> > > > +                                 $fix) {
> > > > +                                     my $line1 = $rawlines[$ctx_ln - 2];
> > >
> > > How are you sure that in a patch context this line always starts with /^\+/ ?
> >
> > Hi,
> > I followed it from the other fixes for OPEN_BRACE which were already
> > there. In the patch context if the lines are added then only I think the fix
> > should be triggered. Other instances should not be modified.
>
> As far as I know there are no existing uses of --fix with OPEN_BRACE.
>

I think you added it via 8d1824780f2f1 ("checkpatch: add --fix option
for a couple OPEN_BRACE misuses")

Thanks,
Dwaipayan.

Re: [PATCH] checkpatch: add --fix option for OPEN_BRACE issues

[Joe Perches]

On Thu, 2020-11-19 at 00:15 +0530, Dwaipayan Ray wrote:
> On Thu, Nov 19, 2020 at 12:09 AM Joe Perches <joe@perches.com> wrote:
> > 
> > On Thu, 2020-11-19 at 00:03 +0530, Dwaipayan Ray wrote:
> > > On Wed, Nov 18, 2020 at 11:44 PM Joe Perches <joe@perches.com> wrote:
> > > > 
> > > > On Wed, 2020-11-18 at 18:10 +0530, Dwaipayan Ray wrote:
> > > > > Brace style misuses of the following types are now
> > > > > corrected:
> > > > []
> > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > > []
> > > > > @@ -3937,9 +3937,23 @@ sub process {
> > > > >                       #print "pre<$pre_ctx>\nline<$line>\nctx<$ctx>\nnext<$lines[$ctx_ln - 1]>\n";
> > > > > 
> > > > > 
> > > > >                       if ($ctx !~ /{\s*/ && defined($lines[$ctx_ln - 1]) && $lines[$ctx_ln - 1] =~ /^\+\s*{/) {
> > > > > -                             ERROR("OPEN_BRACE",
> > > > > -                                   "that open brace { should be on the previous line\n" .
> > > > > -                                     "$here\n$ctx\n$rawlines[$ctx_ln - 1]\n");
> > > > > +                             if (ERROR("OPEN_BRACE",
> > > > > +                                       "that open brace { should be on the previous line\n" .
> > > > > +                                             "$here\n$ctx\n$rawlines[$ctx_ln - 1]\n") &&
> > > > > +                                 $fix) {
> > > > > +                                     my $line1 = $rawlines[$ctx_ln - 2];
> > > > 
> > > > How are you sure that in a patch context this line always starts with /^\+/ ?
> > > 
> > > Hi,
> > > I followed it from the other fixes for OPEN_BRACE which were already
> > > there. In the patch context if the lines are added then only I think the fix
> > > should be triggered. Other instances should not be modified.
> > 
> > As far as I know there are no existing uses of --fix with OPEN_BRACE.
> > 
> 
> I think you added it via 8d1824780f2f1 ("checkpatch: add --fix option
> for a couple OPEN_BRACE misuses")

The difference here is that you are dealing with a $stat context and
the existing --fix entries are just for single line fixes.
	

Re: [PATCH] checkpatch: add --fix option for OPEN_BRACE issues

[Dwaipayan Ray]

On Thu, Nov 19, 2020 at 1:28 AM Joe Perches <joe@perches.com> wrote:
>
> On Thu, 2020-11-19 at 00:15 +0530, Dwaipayan Ray wrote:
> > On Thu, Nov 19, 2020 at 12:09 AM Joe Perches <joe@perches.com> wrote:
> > >
> > > On Thu, 2020-11-19 at 00:03 +0530, Dwaipayan Ray wrote:
> > > > On Wed, Nov 18, 2020 at 11:44 PM Joe Perches <joe@perches.com> wrote:
> > > > >
> > > > > On Wed, 2020-11-18 at 18:10 +0530, Dwaipayan Ray wrote:
> > > > > > Brace style misuses of the following types are now
> > > > > > corrected:
> > > > > []
> > > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > > > []
> > > > > > @@ -3937,9 +3937,23 @@ sub process {
> > > > > >                       #print "pre<$pre_ctx>\nline<$line>\nctx<$ctx>\nnext<$lines[$ctx_ln - 1]>\n";
> > > > > >
> > > > > >
> > > > > >                       if ($ctx !~ /{\s*/ && defined($lines[$ctx_ln - 1]) && $lines[$ctx_ln - 1] =~ /^\+\s*{/) {
> > > > > > -                             ERROR("OPEN_BRACE",
> > > > > > -                                   "that open brace { should be on the previous line\n" .
> > > > > > -                                     "$here\n$ctx\n$rawlines[$ctx_ln - 1]\n");
> > > > > > +                             if (ERROR("OPEN_BRACE",
> > > > > > +                                       "that open brace { should be on the previous line\n" .
> > > > > > +                                             "$here\n$ctx\n$rawlines[$ctx_ln - 1]\n") &&
> > > > > > +                                 $fix) {
> > > > > > +                                     my $line1 = $rawlines[$ctx_ln - 2];
> > > > >
> > > > > How are you sure that in a patch context this line always starts with /^\+/ ?
> > > >
> > > > Hi,
> > > > I followed it from the other fixes for OPEN_BRACE which were already
> > > > there. In the patch context if the lines are added then only I think the fix
> > > > should be triggered. Other instances should not be modified.
> > >
> > > As far as I know there are no existing uses of --fix with OPEN_BRACE.
> > >
> >
> > I think you added it via 8d1824780f2f1 ("checkpatch: add --fix option
> > for a couple OPEN_BRACE misuses")
>
> The difference here is that you are dealing with a $stat context and
> the existing --fix entries are just for single line fixes.
>

Hi,
Ya I understand that. Though I am dealing with $stat content,
I am also directly accessing $rawlines here.
So I think that should have the proper patch line format, starting
with + or - or so.

So in this case if the error is triggered, checking for /^+/ should be done
becase it would be wrong to fix the others with /^[- ]/

Is there something else that I am not getting here?

Thanks,
Dwaipayan.

Re: [PATCH] checkpatch: add --fix option for OPEN_BRACE issues

[Joe Perches]

On Thu, 2020-11-19 at 01:52 +0530, Dwaipayan Ray wrote:
> On Thu, Nov 19, 2020 at 1:28 AM Joe Perches <joe@perches.com> wrote:
> > On Thu, 2020-11-19 at 00:15 +0530, Dwaipayan Ray wrote:
> > > On Thu, Nov 19, 2020 at 12:09 AM Joe Perches <joe@perches.com> wrote:
> > > > On Thu, 2020-11-19 at 00:03 +0530, Dwaipayan Ray wrote:
> > > > > On Wed, Nov 18, 2020 at 11:44 PM Joe Perches <joe@perches.com> wrote:
> > > > > > On Wed, 2020-11-18 at 18:10 +0530, Dwaipayan Ray wrote:
> > > > > > > Brace style misuses of the following types are now
> > > > > > > corrected:
> > > > > > []
> > > > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > > > > []
> > > > > > > @@ -3937,9 +3937,23 @@ sub process {
> > > > > > >                       #print "pre<$pre_ctx>\nline<$line>\nctx<$ctx>\nnext<$lines[$ctx_ln - 1]>\n";
> > > > > > > 
> > > > > > >                       if ($ctx !~ /{\s*/ && defined($lines[$ctx_ln - 1]) && $lines[$ctx_ln - 1] =~ /^\+\s*{/) {
> > > > > > > -                             ERROR("OPEN_BRACE",
> > > > > > > -                                   "that open brace { should be on the previous line\n" .
> > > > > > > -                                     "$here\n$ctx\n$rawlines[$ctx_ln - 1]\n");
> > > > > > > +                             if (ERROR("OPEN_BRACE",
> > > > > > > +                                       "that open brace { should be on the previous line\n" .
> > > > > > > +                                             "$here\n$ctx\n$rawlines[$ctx_ln - 1]\n") &&
> > > > > > > +                                 $fix) {
> > > > > > > +                                     my $line1 = $rawlines[$ctx_ln - 2];
> > > > > > 
> > > > > > How are you sure that in a patch context this line always starts with /^\+/ ?
> > > > > 
> > > > > Hi,
> > > > > I followed it from the other fixes for OPEN_BRACE which were already
> > > > > there. In the patch context if the lines are added then only I think the fix
> > > > > should be triggered. Other instances should not be modified.
> > > > 
> > > > As far as I know there are no existing uses of --fix with OPEN_BRACE.
> > > > 
> > > 
> > > I think you added it via 8d1824780f2f1 ("checkpatch: add --fix option
> > > for a couple OPEN_BRACE misuses")
> > 
> > The difference here is that you are dealing with a $stat context and
> > the existing --fix entries are just for single line fixes.
> > 
> 
> Hi,
> Ya I understand that. Though I am dealing with $stat content,
> I am also directly accessing $rawlines here.
> So I think that should have the proper patch line format, starting
> with + or - or so.
> 
> So in this case if the error is triggered, checking for /^+/ should be done
> becase it would be wrong to fix the others with /^[- ]/
> 
> Is there something else that I am not getting here?

$stat does not include lines that are skipped if the lines start with -

Patch context may be:

line	content

1		func(...
2	-	     original arguments);
3	+	     changed);

where $stat does not include the 'original arguments' changed line

	func(...,
	     changed);

but the $rawlines[] entries are consecutive.

Anyway, this needs to be handled very carefully if handled at all.

I think it's easier to avoid handling these cases and let the
patch submitter fix it manually if appropriate.

Re: [PATCH] checkpatch: add --fix option for OPEN_BRACE issues

[Dwaipayan Ray]

> > > The difference here is that you are dealing with a $stat context and
> > > the existing --fix entries are just for single line fixes.
> > >
> >
> > Hi,
> > Ya I understand that. Though I am dealing with $stat content,
> > I am also directly accessing $rawlines here.
> > So I think that should have the proper patch line format, starting
> > with + or - or so.
> >
> > So in this case if the error is triggered, checking for /^+/ should be done
> > becase it would be wrong to fix the others with /^[- ]/
> >
> > Is there something else that I am not getting here?
>
> $stat does not include lines that are skipped if the lines start with -
>
> Patch context may be:
>
> line    content
>
> 1               func(...
> 2       -            original arguments);
> 3       +            changed);
>
> where $stat does not include the 'original arguments' changed line
>
>         func(...,
>              changed);
>
> but the $rawlines[] entries are consecutive.
>
> Anyway, this needs to be handled very carefully if handled at all.
>
> I think it's easier to avoid handling these cases and let the
> patch submitter fix it manually if appropriate.
>

Thanks. I get your point now. It seems it's much more complex than
I thought it to be. I will avoid working on this one then.

Thanks & Regards,
Dwaipayan.