[PATCH] checkpatch: Fix "Missing a blank line after declarations" test on patches

[PATCH] checkpatch: Fix "Missing a blank line after declarations" test on patches

[Christophe JAILLET]

"Missing a blank line after declarations" is not triggered on patches.
Tweak the regex to match such cases.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This patch is mostly a PoC. I don't know enough about checkpatch.pl to be
sure that the fix is the right thing to do.
At least, it works for me :)

The [\+ ] is taken from the test just above.

I also wonder if there is a missing ^ in the last test:
  (($prevline =~ /[\+ ](\s+)\S/) && $sline =~ /^[\+ ]$1\S/))
                  ^
                  |___ here
---
 scripts/checkpatch.pl | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7b086d1cd6c2..854868b5dbbc 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3783,39 +3783,39 @@ sub process {
 		}
 
 # check for missing blank lines after declarations
-		if ($sline =~ /^\+\s+\S/ &&			#Not at char 1
+		if ($sline =~ /^[\+ ]\s+\S/ &&			#Not at char 1
 			# actual declarations
-		    ($prevline =~ /^\+\s+$Declare\s*$Ident\s*[=,;:\[]/ ||
+		    ($prevline =~ /^[\+ ]\s+$Declare\s*$Ident\s*[=,;:\[]/ ||
 			# function pointer declarations
-		     $prevline =~ /^\+\s+$Declare\s*\(\s*\*\s*$Ident\s*\)\s*[=,;:\[\(]/ ||
+		     $prevline =~ /^[\+ ]\s+$Declare\s*\(\s*\*\s*$Ident\s*\)\s*[=,;:\[\(]/ ||
 			# foo bar; where foo is some local typedef or #define
-		     $prevline =~ /^\+\s+$Ident(?:\s+|\s*\*\s*)$Ident\s*[=,;\[]/ ||
+		     $prevline =~ /^[\+ ]\s+$Ident(?:\s+|\s*\*\s*)$Ident\s*[=,;\[]/ ||
 			# known declaration macros
-		     $prevline =~ /^\+\s+$declaration_macros/) &&
+		     $prevline =~ /^[\+ ]\s+$declaration_macros/) &&
 			# for "else if" which can look like "$Ident $Ident"
-		    !($prevline =~ /^\+\s+$c90_Keywords\b/ ||
+		    !($prevline =~ /^[\+ ]\s+$c90_Keywords\b/ ||
 			# other possible extensions of declaration lines
 		      $prevline =~ /(?:$Compare|$Assignment|$Operators)\s*$/ ||
 			# not starting a section or a macro "\" extended line
 		      $prevline =~ /(?:\{\s*|\\)$/) &&
 			# looks like a declaration
-		    !($sline =~ /^\+\s+$Declare\s*$Ident\s*[=,;:\[]/ ||
+		    !($sline =~ /^[\+ ]\s+$Declare\s*$Ident\s*[=,;:\[]/ ||
 			# function pointer declarations
-		      $sline =~ /^\+\s+$Declare\s*\(\s*\*\s*$Ident\s*\)\s*[=,;:\[\(]/ ||
+		      $sline =~ /^[\+ ]\s+$Declare\s*\(\s*\*\s*$Ident\s*\)\s*[=,;:\[\(]/ ||
 			# foo bar; where foo is some local typedef or #define
-		      $sline =~ /^\+\s+$Ident(?:\s+|\s*\*\s*)$Ident\s*[=,;\[]/ ||
+		      $sline =~ /^[\+ ]\s+$Ident(?:\s+|\s*\*\s*)$Ident\s*[=,;\[]/ ||
 			# known declaration macros
-		      $sline =~ /^\+\s+$declaration_macros/ ||
+		      $sline =~ /^[\+ ]\s+$declaration_macros/ ||
 			# start of struct or union or enum
-		      $sline =~ /^\+\s+(?:static\s+)?(?:const\s+)?(?:union|struct|enum|typedef)\b/ ||
+		      $sline =~ /^[\+ ]\s+(?:static\s+)?(?:const\s+)?(?:union|struct|enum|typedef)\b/ ||
 			# start or end of block or continuation of declaration
-		      $sline =~ /^\+\s+(?:$|[\{\}\.\#\"\?\:\(\[])/ ||
+		      $sline =~ /^[\+ ]\s+(?:$|[\{\}\.\#\"\?\:\(\[])/ ||
 			# bitfield continuation
-		      $sline =~ /^\+\s+$Ident\s*:\s*\d+\s*[,;]/ ||
+		      $sline =~ /^[\+ ]\s+$Ident\s*:\s*\d+\s*[,;]/ ||
 			# other possible extensions of declaration lines
-		      $sline =~ /^\+\s+\(?\s*(?:$Compare|$Assignment|$Operators)/) &&
+		      $sline =~ /^[\+ ]\s+\(?\s*(?:$Compare|$Assignment|$Operators)/) &&
 			# indentation of previous and current line are the same
-		    (($prevline =~ /\+(\s+)\S/) && $sline =~ /^\+$1\S/)) {
+		    (($prevline =~ /[\+ ](\s+)\S/) && $sline =~ /^[\+ ]$1\S/)) {
 			if (WARN("LINE_SPACING",
 				 "Missing a blank line after declarations\n" . $hereprev) &&
 			    $fix) {
-- 
2.27.0

Re: [PATCH] checkpatch: Fix "Missing a blank line after declarations" test on patches

[Joe Perches]

On Thu, 2020-12-10 at 18:52 +0100, Christophe JAILLET wrote:
> "Missing a blank line after declarations" is not triggered on patches.

That's not true.
It does work on any patch that does a new function addition.
There are some patch context complications here when lines are
added and removed such that '+' add, '-' delete, and ' ' context
testing isn't always obvious.

So, the code was intentionally limited to just new functions.

If there are simple ways to avoid false positives, great, but I
believe it's not trivial.

> Tweak the regex to match such cases.

Please send multiple patch examples of different forms where it
does not work.

> This patch is mostly a PoC. I don't know enough about checkpatch.pl to be
> sure that the fix is the right thing to do.
> At least, it works for me :)

Always a starting point...

> 
> The [\+ ] is taken from the test just above.
> 
> I also wonder if there is a missing ^ in the last test:
>   (($prevline =~ /[\+ ](\s+)\S/) && $sline =~ /^[\+ ]$1\S/))
>                   ^
>                   |___ here

Re: [PATCH] checkpatch: Fix "Missing a blank line after declarations" test on patches

[Christophe JAILLET]

Le 10/12/2020 à 19:13, Joe Perches a écrit :
> On Thu, 2020-12-10 at 18:52 +0100, Christophe JAILLET wrote:
>> "Missing a blank line after declarations" is not triggered on patches.
> 
> That's not true.
> It does work on any patch that does a new function addition.
> There are some patch context complications here when lines are
> added and removed such that '+' add, '-' delete, and ' ' context
> testing isn't always obvious.
> 
> So, the code was intentionally limited to just new functions.
> 
> If there are simple ways to avoid false positives, great, but I
> believe it's not trivial.
> 
>> Tweak the regex to match such cases.
> 
> Please send multiple patch examples of different forms where it
> does not work.
> 
>> This patch is mostly a PoC. I don't know enough about checkpatch.pl to be
>> sure that the fix is the right thing to do.
>> At least, it works for me :)
> 
> Always a starting point...
> 
>>
>> The [\+ ] is taken from the test just above.
>>
>> I also wonder if there is a missing ^ in the last test:
>>    (($prevline =~ /[\+ ](\s+)\S/) && $sline =~ /^[\+ ]$1\S/))
>>                    ^
>>                    |___ here
> 
> 
> 
> 

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 10 Dec 2020 14:14:07 +0100

A local variable was used only within an if branch.
Thus move the definition for the variable “mm” into the corresponding
code block.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
  drivers/misc/cxl/cxllib.c | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/misc/cxl/cxllib.c b/drivers/misc/cxl/cxllib.c
index 2a1783f32254..53b919856426 100644
--- a/drivers/misc/cxl/cxllib.c
+++ b/drivers/misc/cxl/cxllib.c
@@ -170,8 +170,6 @@ int cxllib_get_PE_attributes(struct task_struct *task,
  			     unsigned long translation_mode,
  			     struct cxllib_pe_attributes *attr)
  {
-	struct mm_struct *mm = NULL;
-
  	if (translation_mode != CXL_TRANSLATED_MODE &&
  		translation_mode != CXL_REAL_MODE)
  		return -EINVAL;
@@ -182,7 +180,7 @@ int cxllib_get_PE_attributes(struct task_struct *task,
  				true);
  	attr->lpid = mfspr(SPRN_LPID);
  	if (task) {
-		mm = get_task_mm(task);
+		struct mm_struct *mm = get_task_mm(task);
  		if (mm == NULL)
  			return -EINVAL;
  		/*
--
2.29.2