linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] checkpatch: add fix option for ASSIGNMENT_CONTINUATIONS
@ 2020-11-17 17:18 Aditya Srivastava
  2020-11-20 10:41 ` Aditya
  0 siblings, 1 reply; 7+ messages in thread
From: Aditya Srivastava @ 2020-11-17 17:18 UTC (permalink / raw)
  To: joe; +Cc: yashsri421, linux-kernel-mentees, linux-kernel, lukas.bulwahn

Currently, checkpatch warns us if an assignment operator is placed
at the start of a line and not at the end of previous line.

E.g., running checkpatch on commit 8195b1396ec8 ("hv_netvsc: fix
deadlock on hotplug") reports:

CHECK: Assignment operator '=' should be on the previous line
+	struct netvsc_device *nvdev
+		= container_of(w, struct netvsc_device, subchan_work);

Provide a simple fix by appending assignment operator to the previous
line and removing from the current line, if both the lines are additions
(ie start with '+')

Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
---
Changes in v2:
add check if both the lines are additions (ie start with '+')

Changes in v3:
quote $operator; test with division assignment operator ('/=')

 scripts/checkpatch.pl | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c9aaaa443265..d5bc4d8e4f6c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3542,8 +3542,14 @@ sub process {
 
 # check for assignments on the start of a line
 		if ($sline =~ /^\+\s+($Assignment)[^=]/) {
-			CHK("ASSIGNMENT_CONTINUATIONS",
-			    "Assignment operator '$1' should be on the previous line\n" . $hereprev);
+			my $operator = "$1";
+			if (CHK("ASSIGNMENT_CONTINUATIONS",
+				"Assignment operator '$1' should be on the previous line\n" . $hereprev) &&
+			    $fix && $prevrawline =~ /^\+/) {
+				# add assignment operator to the previous line, remove from current line
+				$fixed[$fixlinenr - 1] .= " $operator";
+				$fixed[$fixlinenr] =~ s/$operator\s*//;
+			}
 		}
 
 # check for && or || at the start of a line
-- 
2.17.1


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

* Re: [PATCH v3] checkpatch: add fix option for ASSIGNMENT_CONTINUATIONS
  2020-11-17 17:18 [PATCH v3] checkpatch: add fix option for ASSIGNMENT_CONTINUATIONS Aditya Srivastava
@ 2020-11-20 10:41 ` Aditya
  2020-11-20 17:26   ` Joe Perches
  0 siblings, 1 reply; 7+ messages in thread
From: Aditya @ 2020-11-20 10:41 UTC (permalink / raw)
  To: joe; +Cc: linux-kernel-mentees, linux-kernel, lukas.bulwahn

On 17/11/20 10:48 pm, Aditya Srivastava wrote:
> Currently, checkpatch warns us if an assignment operator is placed
> at the start of a line and not at the end of previous line.
> 
> E.g., running checkpatch on commit 8195b1396ec8 ("hv_netvsc: fix
> deadlock on hotplug") reports:
> 
> CHECK: Assignment operator '=' should be on the previous line
> +	struct netvsc_device *nvdev
> +		= container_of(w, struct netvsc_device, subchan_work);
> 
> Provide a simple fix by appending assignment operator to the previous
> line and removing from the current line, if both the lines are additions
> (ie start with '+')
> 
> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> ---
> Changes in v2:
> add check if both the lines are additions (ie start with '+')
> 
> Changes in v3:
> quote $operator; test with division assignment operator ('/=')
> 
>  scripts/checkpatch.pl | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index c9aaaa443265..d5bc4d8e4f6c 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3542,8 +3542,14 @@ sub process {
>  
>  # check for assignments on the start of a line
>  		if ($sline =~ /^\+\s+($Assignment)[^=]/) {
> -			CHK("ASSIGNMENT_CONTINUATIONS",
> -			    "Assignment operator '$1' should be on the previous line\n" . $hereprev);
> +			my $operator = "$1";
> +			if (CHK("ASSIGNMENT_CONTINUATIONS",
> +				"Assignment operator '$1' should be on the previous line\n" . $hereprev) &&
> +			    $fix && $prevrawline =~ /^\+/) {
> +				# add assignment operator to the previous line, remove from current line
> +				$fixed[$fixlinenr - 1] .= " $operator";
> +				$fixed[$fixlinenr] =~ s/$operator\s*//;
> +			}
>  		}
>  
>  # check for && or || at the start of a line
> 

Hi Joe
This patch probably got missed. Please review :)

Thanks
Aditya

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

* Re: [PATCH v3] checkpatch: add fix option for ASSIGNMENT_CONTINUATIONS
  2020-11-20 10:41 ` Aditya
@ 2020-11-20 17:26   ` Joe Perches
  2020-11-20 18:02     ` Aditya
  0 siblings, 1 reply; 7+ messages in thread
From: Joe Perches @ 2020-11-20 17:26 UTC (permalink / raw)
  To: Aditya; +Cc: linux-kernel-mentees, linux-kernel, lukas.bulwahn

On Fri, 2020-11-20 at 16:11 +0530, Aditya wrote:
> On 17/11/20 10:48 pm, Aditya Srivastava wrote:
> > Currently, checkpatch warns us if an assignment operator is placed
> > at the start of a line and not at the end of previous line.
> > 
> > E.g., running checkpatch on commit 8195b1396ec8 ("hv_netvsc: fix
> > deadlock on hotplug") reports:
> > 
> > CHECK: Assignment operator '=' should be on the previous line
> > +	struct netvsc_device *nvdev
> > +		= container_of(w, struct netvsc_device, subchan_work);
> > 
> > Provide a simple fix by appending assignment operator to the previous
> > line and removing from the current line, if both the lines are additions
> > (ie start with '+')
> > 
> > Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> > ---
> > Changes in v2:
> > add check if both the lines are additions (ie start with '+')
> > 
> > Changes in v3:
> > quote $operator; test with division assignment operator ('/=')
> > 
> >  scripts/checkpatch.pl | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index c9aaaa443265..d5bc4d8e4f6c 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -3542,8 +3542,14 @@ sub process {
> >  
> > 
> >  # check for assignments on the start of a line
> >  		if ($sline =~ /^\+\s+($Assignment)[^=]/) {
> > -			CHK("ASSIGNMENT_CONTINUATIONS",
> > -			    "Assignment operator '$1' should be on the previous line\n" . $hereprev);
> > +			my $operator = "$1";
> > +			if (CHK("ASSIGNMENT_CONTINUATIONS",
> > +				"Assignment operator '$1' should be on the previous line\n" . $hereprev) &&
> > +			    $fix && $prevrawline =~ /^\+/) {
> > +				# add assignment operator to the previous line, remove from current line
> > +				$fixed[$fixlinenr - 1] .= " $operator";
> > +				$fixed[$fixlinenr] =~ s/$operator\s*//;
> > +			}
> >  		}
> >  
> > 
> >  # check for && or || at the start of a line
> > 
> 
> Hi Joe
> This patch probably got missed. Please review :)

Did you look at $Assignment?  Did you see it can be /= ?

If it is, what happens in the $fixed[$fixlinenr] line?



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

* Re: [PATCH v3] checkpatch: add fix option for ASSIGNMENT_CONTINUATIONS
  2020-11-20 17:26   ` Joe Perches
@ 2020-11-20 18:02     ` Aditya
  2020-11-21 11:50       ` Aditya
  0 siblings, 1 reply; 7+ messages in thread
From: Aditya @ 2020-11-20 18:02 UTC (permalink / raw)
  To: Joe Perches; +Cc: linux-kernel-mentees, linux-kernel, lukas.bulwahn

On 20/11/20 10:56 pm, Joe Perches wrote:
> On Fri, 2020-11-20 at 16:11 +0530, Aditya wrote:
>> On 17/11/20 10:48 pm, Aditya Srivastava wrote:
>>> Currently, checkpatch warns us if an assignment operator is placed
>>> at the start of a line and not at the end of previous line.
>>>
>>> E.g., running checkpatch on commit 8195b1396ec8 ("hv_netvsc: fix
>>> deadlock on hotplug") reports:
>>>
>>> CHECK: Assignment operator '=' should be on the previous line
>>> +	struct netvsc_device *nvdev
>>> +		= container_of(w, struct netvsc_device, subchan_work);
>>>
>>> Provide a simple fix by appending assignment operator to the previous
>>> line and removing from the current line, if both the lines are additions
>>> (ie start with '+')
>>>
>>> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
>>> ---
>>> Changes in v2:
>>> add check if both the lines are additions (ie start with '+')
>>>
>>> Changes in v3:
>>> quote $operator; test with division assignment operator ('/=')
>>>
>>>  scripts/checkpatch.pl | 10 ++++++++--
>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>> index c9aaaa443265..d5bc4d8e4f6c 100755
>>> --- a/scripts/checkpatch.pl
>>> +++ b/scripts/checkpatch.pl
>>> @@ -3542,8 +3542,14 @@ sub process {
>>>  
>>>
>>>  # check for assignments on the start of a line
>>>  		if ($sline =~ /^\+\s+($Assignment)[^=]/) {
>>> -			CHK("ASSIGNMENT_CONTINUATIONS",
>>> -			    "Assignment operator '$1' should be on the previous line\n" . $hereprev);
>>> +			my $operator = "$1";
>>> +			if (CHK("ASSIGNMENT_CONTINUATIONS",
>>> +				"Assignment operator '$1' should be on the previous line\n" . $hereprev) &&
>>> +			    $fix && $prevrawline =~ /^\+/) {
>>> +				# add assignment operator to the previous line, remove from current line
>>> +				$fixed[$fixlinenr - 1] .= " $operator";
>>> +				$fixed[$fixlinenr] =~ s/$operator\s*//;
>>> +			}
>>>  		}
>>>  
>>>
>>>  # check for && or || at the start of a line
>>>
>>
>> Hi Joe
>> This patch probably got missed. Please review :)
> 
> Did you look at $Assignment?  Did you see it can be /= ?
> 

Yes, I tested the patch with '/=' operator as well.
As I could not find any occurrences in the past(over 4.13..5.8), I
created an example for myself by modifying the above mentioned commit
example i.e. commit 8195b1396ec8 ("hv_netvsc: fix deadlock on
hotplug") as:

+	struct netvsc_device *nvdev
+		/= container_of(w, struct netvsc_device, subchan_work);

[For Line 144 and 145(where the warning was reported for '=' earlier)]

The fix changes these lines to:
+	struct netvsc_device *nvdev /=
+		container_of(w, struct netvsc_device, subchan_work);

On retesting the patch with checkpatch.pl, it did not give this CHECK,
nor did we add any new warning/error.

> If it is, what happens in the $fixed[$fixlinenr] line?
>

In $fixed[$fixlinenr], we are just getting rid of the operator and any
space(s) following it.

What do you think?

Thanks
Aditya

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

* Re: [PATCH v3] checkpatch: add fix option for ASSIGNMENT_CONTINUATIONS
  2020-11-20 18:02     ` Aditya
@ 2020-11-21 11:50       ` Aditya
  2020-11-21 12:04         ` [PATCH v4] " Aditya Srivastava
  0 siblings, 1 reply; 7+ messages in thread
From: Aditya @ 2020-11-21 11:50 UTC (permalink / raw)
  To: Joe Perches; +Cc: linux-kernel-mentees, linux-kernel, lukas.bulwahn

On 20/11/20 11:32 pm, Aditya wrote:
> On 20/11/20 10:56 pm, Joe Perches wrote:
>> On Fri, 2020-11-20 at 16:11 +0530, Aditya wrote:
>>> On 17/11/20 10:48 pm, Aditya Srivastava wrote:
>>>> Currently, checkpatch warns us if an assignment operator is placed
>>>> at the start of a line and not at the end of previous line.
>>>>
>>>> E.g., running checkpatch on commit 8195b1396ec8 ("hv_netvsc: fix
>>>> deadlock on hotplug") reports:
>>>>
>>>> CHECK: Assignment operator '=' should be on the previous line
>>>> +	struct netvsc_device *nvdev
>>>> +		= container_of(w, struct netvsc_device, subchan_work);
>>>>
>>>> Provide a simple fix by appending assignment operator to the previous
>>>> line and removing from the current line, if both the lines are additions
>>>> (ie start with '+')
>>>>
>>>> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
>>>> ---
>>>> Changes in v2:
>>>> add check if both the lines are additions (ie start with '+')
>>>>
>>>> Changes in v3:
>>>> quote $operator; test with division assignment operator ('/=')
>>>>
>>>>  scripts/checkpatch.pl | 10 ++++++++--
>>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>>> index c9aaaa443265..d5bc4d8e4f6c 100755
>>>> --- a/scripts/checkpatch.pl
>>>> +++ b/scripts/checkpatch.pl
>>>> @@ -3542,8 +3542,14 @@ sub process {
>>>>  
>>>>
>>>>  # check for assignments on the start of a line
>>>>  		if ($sline =~ /^\+\s+($Assignment)[^=]/) {
>>>> -			CHK("ASSIGNMENT_CONTINUATIONS",
>>>> -			    "Assignment operator '$1' should be on the previous line\n" . $hereprev);
>>>> +			my $operator = "$1";
>>>> +			if (CHK("ASSIGNMENT_CONTINUATIONS",
>>>> +				"Assignment operator '$1' should be on the previous line\n" . $hereprev) &&
>>>> +			    $fix && $prevrawline =~ /^\+/) {
>>>> +				# add assignment operator to the previous line, remove from current line
>>>> +				$fixed[$fixlinenr - 1] .= " $operator";
>>>> +				$fixed[$fixlinenr] =~ s/$operator\s*//;
>>>> +			}
>>>>  		}
>>>>  
>>>>
>>>>  # check for && or || at the start of a line
>>>>
>>>
>>> Hi Joe
>>> This patch probably got missed. Please review :)
>>
>> Did you look at $Assignment?  Did you see it can be /= ?
>>
> 
> Yes, I tested the patch with '/=' operator as well.
> As I could not find any occurrences in the past(over 4.13..5.8), I
> created an example for myself by modifying the above mentioned commit
> example i.e. commit 8195b1396ec8 ("hv_netvsc: fix deadlock on
> hotplug") as:
> 
> +	struct netvsc_device *nvdev
> +		/= container_of(w, struct netvsc_device, subchan_work);
> 
> [For Line 144 and 145(where the warning was reported for '=' earlier)]
> 
> The fix changes these lines to:
> +	struct netvsc_device *nvdev /=
> +		container_of(w, struct netvsc_device, subchan_work);
> 
> On retesting the patch with checkpatch.pl, it did not give this CHECK,
> nor did we add any new warning/error.
> 
>> If it is, what happens in the $fixed[$fixlinenr] line?
>>
> 
> In $fixed[$fixlinenr], we are just getting rid of the operator and any
> space(s) following it.
> 
> What do you think?
> 

I think I understood quoting incorrectly here. I had to use \Q.
I'll resend the modified patch with desired changes.
Sorry for inconvenience.

Thanks
Aditya

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

* [PATCH v4] checkpatch: add fix option for ASSIGNMENT_CONTINUATIONS
  2020-11-21 11:50       ` Aditya
@ 2020-11-21 12:04         ` Aditya Srivastava
  2020-11-21 12:10           ` Joe Perches
  0 siblings, 1 reply; 7+ messages in thread
From: Aditya Srivastava @ 2020-11-21 12:04 UTC (permalink / raw)
  To: joe; +Cc: yashsri421, lukas.bulwahn, linux-kernel-mentees, linux-kernel

Currently, checkpatch warns us if an assignment operator is placed
at the start of a line and not at the end of previous line.

E.g., running checkpatch on commit 8195b1396ec8 ("hv_netvsc: fix
deadlock on hotplug") reports:

CHECK: Assignment operator '=' should be on the previous line
+	struct netvsc_device *nvdev
+		= container_of(w, struct netvsc_device, subchan_work);

Provide a simple fix by appending assignment operator to the previous
line and removing from the current line, if both the lines are additions
(ie start with '+')

Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
---
Changes in v2:
add check if both the lines are additions (ie start with '+')

Changes in v3:
quote $operator; test with division assignment operator ('/=')

Changes in v4:
fix incorrect use of quote

 scripts/checkpatch.pl | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2749f32dffe9..d4c8d42cb13e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3533,8 +3533,14 @@ sub process {
 
 # check for assignments on the start of a line
 		if ($sline =~ /^\+\s+($Assignment)[^=]/) {
-			CHK("ASSIGNMENT_CONTINUATIONS",
-			    "Assignment operator '$1' should be on the previous line\n" . $hereprev);
+			my $operator = $1;
+			if (CHK("ASSIGNMENT_CONTINUATIONS",
+				"Assignment operator '$1' should be on the previous line\n" . $hereprev) &&
+			    $fix && $prevrawline =~ /^\+/) {
+				# add assignment operator to the previous line, remove from current line
+				$fixed[$fixlinenr - 1] .= " $operator";
+				$fixed[$fixlinenr] =~ s/\Q$operator\E\s*//;
+			}
 		}
 
 # check for && or || at the start of a line
-- 
2.17.1


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

* Re: [PATCH v4] checkpatch: add fix option for ASSIGNMENT_CONTINUATIONS
  2020-11-21 12:04         ` [PATCH v4] " Aditya Srivastava
@ 2020-11-21 12:10           ` Joe Perches
  0 siblings, 0 replies; 7+ messages in thread
From: Joe Perches @ 2020-11-21 12:10 UTC (permalink / raw)
  To: Aditya Srivastava, Andrew Morton
  Cc: lukas.bulwahn, linux-kernel-mentees, linux-kernel

On Sat, 2020-11-21 at 17:34 +0530, Aditya Srivastava wrote:
> Currently, checkpatch warns us if an assignment operator is placed
> at the start of a line and not at the end of previous line.

Right, thanks.

Acked-by: Joe Perches <joe@perches.com>

> 
> E.g., running checkpatch on commit 8195b1396ec8 ("hv_netvsc: fix
> deadlock on hotplug") reports:
> 
> CHECK: Assignment operator '=' should be on the previous line
> +	struct netvsc_device *nvdev
> +		= container_of(w, struct netvsc_device, subchan_work);
> 
> Provide a simple fix by appending assignment operator to the previous
> line and removing from the current line, if both the lines are additions
> (ie start with '+')
> 
> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> ---
> Changes in v2:
> add check if both the lines are additions (ie start with '+')
> 
> Changes in v3:
> quote $operator; test with division assignment operator ('/=')
> 
> Changes in v4:
> fix incorrect use of quote
> 
>  scripts/checkpatch.pl | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 2749f32dffe9..d4c8d42cb13e 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3533,8 +3533,14 @@ sub process {
>  
> 
>  # check for assignments on the start of a line
>  		if ($sline =~ /^\+\s+($Assignment)[^=]/) {
> -			CHK("ASSIGNMENT_CONTINUATIONS",
> -			    "Assignment operator '$1' should be on the previous line\n" . $hereprev);
> +			my $operator = $1;
> +			if (CHK("ASSIGNMENT_CONTINUATIONS",
> +				"Assignment operator '$1' should be on the previous line\n" . $hereprev) &&
> +			    $fix && $prevrawline =~ /^\+/) {
> +				# add assignment operator to the previous line, remove from current line
> +				$fixed[$fixlinenr - 1] .= " $operator";
> +				$fixed[$fixlinenr] =~ s/\Q$operator\E\s*//;
> +			}
>  		}
>  
> 
>  # check for && or || at the start of a line



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

end of thread, other threads:[~2020-11-21 12:10 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-17 17:18 [PATCH v3] checkpatch: add fix option for ASSIGNMENT_CONTINUATIONS Aditya Srivastava
2020-11-20 10:41 ` Aditya
2020-11-20 17:26   ` Joe Perches
2020-11-20 18:02     ` Aditya
2020-11-21 11:50       ` Aditya
2020-11-21 12:04         ` [PATCH v4] " Aditya Srivastava
2020-11-21 12:10           ` 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).