[PATCH v5] checkpatch: extend attributes check to handle more patterns

[PATCH v5] checkpatch: extend attributes check to handle more patterns

[Dwaipayan Ray]

It is generally preferred that the macros from
include/linux/compiler_attributes.h are used, unless there
is a reason not to.

checkpatch currently checks __attribute__ for each of
packed, aligned, section, printf, scanf, and weak. Other
declarations in compiler_attributes.h are not handled.

Add a generic test to check the presence of such attributes.
Some attributes require more specific handling and are kept
separate.

Also add fixes to the generic attributes check to substitute
the correct conversions.

New attributes which are now handled are:

__always_inline__
__assume_aligned__(a, ## __VA_ARGS__)
__cold__
__const__
__copy__(symbol)
__designated_init__
__externally_visible__
__gnu_inline__
__malloc__
__mode__(x)
__no_caller_saved_registers__
__noclone__
__noinline__
__nonstring__
__noreturn__
__pure__
__unused__
__used__

Declarations which contain multiple attributes like
__attribute__((__packed__, __cold__)) are also handled except
when proper conversions for one or more attributes of the list
cannot be determined.

Link: https://lore.kernel.org/linux-kernel-mentees/3ec15b41754b01666d94b76ce51b9832c2dd577a.camel@perches.com/
Suggested-by: Joe Perches <joe@perches.com>
Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
---
 scripts/checkpatch.pl | 117 +++++++++++++++++++++++++++++-------------
 1 file changed, 81 insertions(+), 36 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7e505688257a..e3437948883b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6155,50 +6155,95 @@ sub process {
 			}
 		}
 
-# Check for __attribute__ packed, prefer __packed
+# Check for compiler attributes
 		if ($realfile !~ m@\binclude/uapi/@ &&
-		    $line =~ /\b__attribute__\s*\(\s*\(.*\bpacked\b/) {
-			WARN("PREFER_PACKED",
-			     "__packed is preferred over __attribute__((packed))\n" . $herecurr);
-		}
+		    $rawline =~ /\b__attribute__\s*\(\s*($balanced_parens)\s*\)/) {
+			my $attr = $1;
+			$attr =~ s/\s*\(\s*(.*)\)\s*/$1/;
+
+			my %attr_list = (
+				"aligned"			=> "__aligned",
+				"always_inline"			=> "__always_inline",
+				"assume_aligned"		=> "__assume_aligned",
+				"cold"				=> "__cold",
+				"const"				=> "__const",
+				"copy"				=> "__copy",
+				"designated_init"		=> "__designated_init",
+				"externally_visible"		=> "__visible",
+				"gnu_inline"			=> "__gnu_inline",
+				"malloc"			=> "__malloc",
+				"mode"				=> "__mode",
+				"no_caller_saved_registers"	=> "__no_caller_saved_registers",
+				"noclone"			=> "__noclone",
+				"noinline"			=> "noinline",
+				"nonstring"			=> "__nonstring",
+				"noreturn"			=> "__noreturn",
+				"packed"			=> "__packed",
+				"pure"				=> "__pure",
+				"used"				=> "__used"
+			);
+
+			my @conv_array = ();
+			my $conv_possible = 1;
+
+			while ($attr =~ /\s*(\w+)\s*(${balanced_parens})?/g) {
+				my $curr_attr = $1;
+				my $params = '';
+				$params = $2 if defined($2);
+				$curr_attr =~ s/^[\s_]+|[\s_]+$//g;
+
+				if (exists($attr_list{$curr_attr})) {
+					my $new = $attr_list{$curr_attr};
+					push(@conv_array, "$new$params");
+				} else {
+					$conv_possible = 0;
+					last;
+				}
+			}
 
-# Check for __attribute__ aligned, prefer __aligned
-		if ($realfile !~ m@\binclude/uapi/@ &&
-		    $line =~ /\b__attribute__\s*\(\s*\(.*aligned/) {
-			WARN("PREFER_ALIGNED",
-			     "__aligned(size) is preferred over __attribute__((aligned(size)))\n" . $herecurr);
-		}
+			if (scalar @conv_array > 0 && $conv_possible == 1) {
+				my $replace = join(' ', @conv_array);
+				if (WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
+				         "$replace is preferred over __attribute__(($attr))\n" . $herecurr) &&
+					$fix) {
+					$fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*\Q$attr\E\s*\)\s*\)/$replace/;
+				}
+			}
 
-# Check for __attribute__ section, prefer __section
-		if ($realfile !~ m@\binclude/uapi/@ &&
-		    $line =~ /\b__attribute__\s*\(\s*\(.*_*section_*\s*\(\s*("[^"]*")/) {
-			my $old = substr($rawline, $-[1], $+[1] - $-[1]);
-			my $new = substr($old, 1, -1);
-			if (WARN("PREFER_SECTION",
-				 "__section($new) is preferred over __attribute__((section($old)))\n" . $herecurr) &&
-			    $fix) {
-				$fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*_*section_*\s*\(\s*\Q$old\E\s*\)\s*\)\s*\)/__section($new)/;
+			# Check for __attribute__ format(printf, prefer __printf
+			if ($attr =~ /^_*format_*\s*\(\s*printf/) {
+				if (WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
+				         "__printf(string-index, first-to-check) is preferred over __attribute__((format(printf, string-index, first-to-check)))\n" . $herecurr) &&
+					$fix) {
+					$fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*_*format_*\s*\(\s*printf\s*,\s*(.*)\)\s*\)\s*\)/"__printf(" . trim($1) . ")"/ex;
+
+				}
 			}
-		}
 
-# Check for __attribute__ format(printf, prefer __printf
-		if ($realfile !~ m@\binclude/uapi/@ &&
-		    $line =~ /\b__attribute__\s*\(\s*\(\s*format\s*\(\s*printf/) {
-			if (WARN("PREFER_PRINTF",
-				 "__printf(string-index, first-to-check) is preferred over __attribute__((format(printf, string-index, first-to-check)))\n" . $herecurr) &&
-			    $fix) {
-				$fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*format\s*\(\s*printf\s*,\s*(.*)\)\s*\)\s*\)/"__printf(" . trim($1) . ")"/ex;
+			# Check for __attribute__ format(scanf, prefer __scanf
+			if ($attr =~ /^_*format_*\s*\(\s*scanf\b/) {
+				if (WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
+				         "__scanf(string-index, first-to-check) is preferred over __attribute__((format(scanf, string-index, first-to-check)))\n" . $herecurr) &&
+					$fix) {
+					$fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*_*format_*\s*\(\s*scanf\s*,\s*(.*)\)\s*\)\s*\)/"__scanf(" . trim($1) . ")"/ex;
+				}
+			}
 
+			# Check for __attribute__ section, prefer __section
+			if ($attr =~ /^_*section_*\s*\(\s*("[^"]*")/) {
+				my $old = substr($attr, $-[1], $+[1] - $-[1]);
+				my $new = substr($old, 1, -1);
+				if (WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
+				         "__section($new) is preferred over __attribute__((section($old)))\n" . $herecurr) &&
+					$fix) {
+					$fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*_*section_*\s*\(\s*\Q$old\E\s*\)\s*\)\s*\)/__section($new)/;
+				}
 			}
-		}
 
-# Check for __attribute__ format(scanf, prefer __scanf
-		if ($realfile !~ m@\binclude/uapi/@ &&
-		    $line =~ /\b__attribute__\s*\(\s*\(\s*format\s*\(\s*scanf\b/) {
-			if (WARN("PREFER_SCANF",
-				 "__scanf(string-index, first-to-check) is preferred over __attribute__((format(scanf, string-index, first-to-check)))\n" . $herecurr) &&
-			    $fix) {
-				$fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*format\s*\(\s*scanf\s*,\s*(.*)\)\s*\)\s*\)/"__scanf(" . trim($1) . ")"/ex;
+			# Check for __attribute__ unused, prefer __always_unused or __maybe_unused
+			if ($attr =~ /^_*unused/) {
+				WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
+				     "__always_unused or __maybe_unused is preferred over __attribute__((__unused__))\n" . $herecurr);
 			}
 		}
 
-- 
2.27.0

[Linux-kernel-mentees] [PATCH v5] checkpatch: extend attributes check to handle more patterns

[Dwaipayan Ray]

It is generally preferred that the macros from
include/linux/compiler_attributes.h are used, unless there
is a reason not to.

checkpatch currently checks __attribute__ for each of
packed, aligned, section, printf, scanf, and weak. Other
declarations in compiler_attributes.h are not handled.

Add a generic test to check the presence of such attributes.
Some attributes require more specific handling and are kept
separate.

Also add fixes to the generic attributes check to substitute
the correct conversions.

New attributes which are now handled are:

__always_inline__
__assume_aligned__(a, ## __VA_ARGS__)
__cold__
__const__
__copy__(symbol)
__designated_init__
__externally_visible__
__gnu_inline__
__malloc__
__mode__(x)
__no_caller_saved_registers__
__noclone__
__noinline__
__nonstring__
__noreturn__
__pure__
__unused__
__used__

Declarations which contain multiple attributes like
__attribute__((__packed__, __cold__)) are also handled except
when proper conversions for one or more attributes of the list
cannot be determined.

Link: https://lore.kernel.org/linux-kernel-mentees/3ec15b41754b01666d94b76ce51b9832c2dd577a.camel@perches.com/
Suggested-by: Joe Perches <joe@perches.com>
Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
---
 scripts/checkpatch.pl | 117 +++++++++++++++++++++++++++++-------------
 1 file changed, 81 insertions(+), 36 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7e505688257a..e3437948883b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6155,50 +6155,95 @@ sub process {
 			}
 		}
 
-# Check for __attribute__ packed, prefer __packed
+# Check for compiler attributes
 		if ($realfile !~ m@\binclude/uapi/@ &&
-		    $line =~ /\b__attribute__\s*\(\s*\(.*\bpacked\b/) {
-			WARN("PREFER_PACKED",
-			     "__packed is preferred over __attribute__((packed))\n" . $herecurr);
-		}
+		    $rawline =~ /\b__attribute__\s*\(\s*($balanced_parens)\s*\)/) {
+			my $attr = $1;
+			$attr =~ s/\s*\(\s*(.*)\)\s*/$1/;
+
+			my %attr_list = (
+				"aligned"			=> "__aligned",
+				"always_inline"			=> "__always_inline",
+				"assume_aligned"		=> "__assume_aligned",
+				"cold"				=> "__cold",
+				"const"				=> "__const",
+				"copy"				=> "__copy",
+				"designated_init"		=> "__designated_init",
+				"externally_visible"		=> "__visible",
+				"gnu_inline"			=> "__gnu_inline",
+				"malloc"			=> "__malloc",
+				"mode"				=> "__mode",
+				"no_caller_saved_registers"	=> "__no_caller_saved_registers",
+				"noclone"			=> "__noclone",
+				"noinline"			=> "noinline",
+				"nonstring"			=> "__nonstring",
+				"noreturn"			=> "__noreturn",
+				"packed"			=> "__packed",
+				"pure"				=> "__pure",
+				"used"				=> "__used"
+			);
+
+			my @conv_array = ();
+			my $conv_possible = 1;
+
+			while ($attr =~ /\s*(\w+)\s*(${balanced_parens})?/g) {
+				my $curr_attr = $1;
+				my $params = '';
+				$params = $2 if defined($2);
+				$curr_attr =~ s/^[\s_]+|[\s_]+$//g;
+
+				if (exists($attr_list{$curr_attr})) {
+					my $new = $attr_list{$curr_attr};
+					push(@conv_array, "$new$params");
+				} else {
+					$conv_possible = 0;
+					last;
+				}
+			}
 
-# Check for __attribute__ aligned, prefer __aligned
-		if ($realfile !~ m@\binclude/uapi/@ &&
-		    $line =~ /\b__attribute__\s*\(\s*\(.*aligned/) {
-			WARN("PREFER_ALIGNED",
-			     "__aligned(size) is preferred over __attribute__((aligned(size)))\n" . $herecurr);
-		}
+			if (scalar @conv_array > 0 && $conv_possible == 1) {
+				my $replace = join(' ', @conv_array);
+				if (WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
+				         "$replace is preferred over __attribute__(($attr))\n" . $herecurr) &&
+					$fix) {
+					$fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*\Q$attr\E\s*\)\s*\)/$replace/;
+				}
+			}
 
-# Check for __attribute__ section, prefer __section
-		if ($realfile !~ m@\binclude/uapi/@ &&
-		    $line =~ /\b__attribute__\s*\(\s*\(.*_*section_*\s*\(\s*("[^"]*")/) {
-			my $old = substr($rawline, $-[1], $+[1] - $-[1]);
-			my $new = substr($old, 1, -1);
-			if (WARN("PREFER_SECTION",
-				 "__section($new) is preferred over __attribute__((section($old)))\n" . $herecurr) &&
-			    $fix) {
-				$fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*_*section_*\s*\(\s*\Q$old\E\s*\)\s*\)\s*\)/__section($new)/;
+			# Check for __attribute__ format(printf, prefer __printf
+			if ($attr =~ /^_*format_*\s*\(\s*printf/) {
+				if (WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
+				         "__printf(string-index, first-to-check) is preferred over __attribute__((format(printf, string-index, first-to-check)))\n" . $herecurr) &&
+					$fix) {
+					$fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*_*format_*\s*\(\s*printf\s*,\s*(.*)\)\s*\)\s*\)/"__printf(" . trim($1) . ")"/ex;
+
+				}
 			}
-		}
 
-# Check for __attribute__ format(printf, prefer __printf
-		if ($realfile !~ m@\binclude/uapi/@ &&
-		    $line =~ /\b__attribute__\s*\(\s*\(\s*format\s*\(\s*printf/) {
-			if (WARN("PREFER_PRINTF",
-				 "__printf(string-index, first-to-check) is preferred over __attribute__((format(printf, string-index, first-to-check)))\n" . $herecurr) &&
-			    $fix) {
-				$fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*format\s*\(\s*printf\s*,\s*(.*)\)\s*\)\s*\)/"__printf(" . trim($1) . ")"/ex;
+			# Check for __attribute__ format(scanf, prefer __scanf
+			if ($attr =~ /^_*format_*\s*\(\s*scanf\b/) {
+				if (WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
+				         "__scanf(string-index, first-to-check) is preferred over __attribute__((format(scanf, string-index, first-to-check)))\n" . $herecurr) &&
+					$fix) {
+					$fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*_*format_*\s*\(\s*scanf\s*,\s*(.*)\)\s*\)\s*\)/"__scanf(" . trim($1) . ")"/ex;
+				}
+			}
 
+			# Check for __attribute__ section, prefer __section
+			if ($attr =~ /^_*section_*\s*\(\s*("[^"]*")/) {
+				my $old = substr($attr, $-[1], $+[1] - $-[1]);
+				my $new = substr($old, 1, -1);
+				if (WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
+				         "__section($new) is preferred over __attribute__((section($old)))\n" . $herecurr) &&
+					$fix) {
+					$fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*_*section_*\s*\(\s*\Q$old\E\s*\)\s*\)\s*\)/__section($new)/;
+				}
 			}
-		}
 
-# Check for __attribute__ format(scanf, prefer __scanf
-		if ($realfile !~ m@\binclude/uapi/@ &&
-		    $line =~ /\b__attribute__\s*\(\s*\(\s*format\s*\(\s*scanf\b/) {
-			if (WARN("PREFER_SCANF",
-				 "__scanf(string-index, first-to-check) is preferred over __attribute__((format(scanf, string-index, first-to-check)))\n" . $herecurr) &&
-			    $fix) {
-				$fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*format\s*\(\s*scanf\s*,\s*(.*)\)\s*\)\s*\)/"__scanf(" . trim($1) . ")"/ex;
+			# Check for __attribute__ unused, prefer __always_unused or __maybe_unused
+			if ($attr =~ /^_*unused/) {
+				WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
+				     "__always_unused or __maybe_unused is preferred over __attribute__((__unused__))\n" . $herecurr);
 			}
 		}
 
-- 
2.27.0

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH v5] checkpatch: extend attributes check to handle more patterns

[Joe Perches]

On Sun, 2020-10-25 at 15:45 +0530, Dwaipayan Ray wrote:
> It is generally preferred that the macros from
> include/linux/compiler_attributes.h are used, unless there
> is a reason not to.
> 
> checkpatch currently checks __attribute__ for each of
> packed, aligned, section, printf, scanf, and weak. Other
> declarations in compiler_attributes.h are not handled.
> 
> Add a generic test to check the presence of such attributes.
> Some attributes require more specific handling and are kept
> separate.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -6155,50 +6155,95 @@ sub process {
>  			}
>  		}
>  
> 
> -# Check for __attribute__ packed, prefer __packed
> +# Check for compiler attributes
>  		if ($realfile !~ m@\binclude/uapi/@ &&
> -		    $line =~ /\b__attribute__\s*\(\s*\(.*\bpacked\b/) {
> -			WARN("PREFER_PACKED",
> -			     "__packed is preferred over __attribute__((packed))\n" . $herecurr);
> -		}
> +		    $rawline =~ /\b__attribute__\s*\(\s*($balanced_parens)\s*\)/) {

Using $rawline would also change comments and that seems wrong.
Any reason to use $rawline instead of $line?

[]

> +			if (scalar @conv_array > 0 && $conv_possible == 1) {
> +				my $replace = join(' ', @conv_array);
> +				if (WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
> +				         "$replace is preferred over __attribute__(($attr))\n" . $herecurr) &&
> +					$fix) {
> +					$fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*\Q$attr\E\s*\)\s*\)/$replace/;

I looks it would be useful to add
					$fixed[$fixlinenr] =~ s/\}\Q$replace\E/} $replace/;
so there's a space added between } and any replacements.

Re: [Linux-kernel-mentees] [PATCH v5] checkpatch: extend attributes check to handle more patterns

[Joe Perches]

On Sun, 2020-10-25 at 15:45 +0530, Dwaipayan Ray wrote:
> It is generally preferred that the macros from
> include/linux/compiler_attributes.h are used, unless there
> is a reason not to.
> 
> checkpatch currently checks __attribute__ for each of
> packed, aligned, section, printf, scanf, and weak. Other
> declarations in compiler_attributes.h are not handled.
> 
> Add a generic test to check the presence of such attributes.
> Some attributes require more specific handling and are kept
> separate.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -6155,50 +6155,95 @@ sub process {
>  			}
>  		}
>  
> 
> -# Check for __attribute__ packed, prefer __packed
> +# Check for compiler attributes
>  		if ($realfile !~ m@\binclude/uapi/@ &&
> -		    $line =~ /\b__attribute__\s*\(\s*\(.*\bpacked\b/) {
> -			WARN("PREFER_PACKED",
> -			     "__packed is preferred over __attribute__((packed))\n" . $herecurr);
> -		}
> +		    $rawline =~ /\b__attribute__\s*\(\s*($balanced_parens)\s*\)/) {

Using $rawline would also change comments and that seems wrong.
Any reason to use $rawline instead of $line?

[]

> +			if (scalar @conv_array > 0 && $conv_possible == 1) {
> +				my $replace = join(' ', @conv_array);
> +				if (WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
> +				         "$replace is preferred over __attribute__(($attr))\n" . $herecurr) &&
> +					$fix) {
> +					$fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*\Q$attr\E\s*\)\s*\)/$replace/;

I looks it would be useful to add
					$fixed[$fixlinenr] =~ s/\}\Q$replace\E/} $replace/;
so there's a space added between } and any replacements.


_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH v5] checkpatch: extend attributes check to handle more patterns

[Dwaipayan Ray]

On Sun, Oct 25, 2020 at 11:29 PM Joe Perches <joe@perches.com> wrote:
>
> On Sun, 2020-10-25 at 15:45 +0530, Dwaipayan Ray wrote:
> > It is generally preferred that the macros from
> > include/linux/compiler_attributes.h are used, unless there
> > is a reason not to.
> >
> > checkpatch currently checks __attribute__ for each of
> > packed, aligned, section, printf, scanf, and weak. Other
> > declarations in compiler_attributes.h are not handled.
> >
> > Add a generic test to check the presence of such attributes.
> > Some attributes require more specific handling and are kept
> > separate.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -6155,50 +6155,95 @@ sub process {
> >                       }
> >               }
> >
> >
> > -# Check for __attribute__ packed, prefer __packed
> > +# Check for compiler attributes
> >               if ($realfile !~ m@\binclude/uapi/@ &&
> > -                 $line =~ /\b__attribute__\s*\(\s*\(.*\bpacked\b/) {
> > -                     WARN("PREFER_PACKED",
> > -                          "__packed is preferred over __attribute__((packed))\n" . $herecurr);
> > -             }
> > +                 $rawline =~ /\b__attribute__\s*\(\s*($balanced_parens)\s*\)/) {
>
> Using $rawline would also change comments and that seems wrong.
> Any reason to use $rawline instead of $line?
>
Hi,
Yes I used $line initially but changed it because quoted
strings were being replaced.

Like:
__attribute__((__section__("_ftrace_events")))

$line in this case was:
__attribute__((__section__("XXXXXXXXXXXXXX")))

While $rawline was:
__attribute__((__section__("_ftrace_events")))

So to avoid this problem I changed to $rawline.

Is there any other alternative available perhaps?
Or should I change back to $rawline?

> []
>
> > +                     if (scalar @conv_array > 0 && $conv_possible == 1) {
> > +                             my $replace = join(' ', @conv_array);
> > +                             if (WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
> > +                                      "$replace is preferred over __attribute__(($attr))\n" . $herecurr) &&
> > +                                     $fix) {
> > +                                     $fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*\Q$attr\E\s*\)\s*\)/$replace/;
>
> I looks it would be useful to add
>                                         $fixed[$fixlinenr] =~ s/\}\Q$replace\E/} $replace/;
> so there's a space added between } and any replacements.
>
Yes I will do that sure.

Thanks,
Dwaipayan.

Re: [Linux-kernel-mentees] [PATCH v5] checkpatch: extend attributes check to handle more patterns

[Dwaipayan Ray]

On Sun, Oct 25, 2020 at 11:29 PM Joe Perches <joe@perches.com> wrote:
>
> On Sun, 2020-10-25 at 15:45 +0530, Dwaipayan Ray wrote:
> > It is generally preferred that the macros from
> > include/linux/compiler_attributes.h are used, unless there
> > is a reason not to.
> >
> > checkpatch currently checks __attribute__ for each of
> > packed, aligned, section, printf, scanf, and weak. Other
> > declarations in compiler_attributes.h are not handled.
> >
> > Add a generic test to check the presence of such attributes.
> > Some attributes require more specific handling and are kept
> > separate.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -6155,50 +6155,95 @@ sub process {
> >                       }
> >               }
> >
> >
> > -# Check for __attribute__ packed, prefer __packed
> > +# Check for compiler attributes
> >               if ($realfile !~ m@\binclude/uapi/@ &&
> > -                 $line =~ /\b__attribute__\s*\(\s*\(.*\bpacked\b/) {
> > -                     WARN("PREFER_PACKED",
> > -                          "__packed is preferred over __attribute__((packed))\n" . $herecurr);
> > -             }
> > +                 $rawline =~ /\b__attribute__\s*\(\s*($balanced_parens)\s*\)/) {
>
> Using $rawline would also change comments and that seems wrong.
> Any reason to use $rawline instead of $line?
>
Hi,
Yes I used $line initially but changed it because quoted
strings were being replaced.

Like:
__attribute__((__section__("_ftrace_events")))

$line in this case was:
__attribute__((__section__("XXXXXXXXXXXXXX")))

While $rawline was:
__attribute__((__section__("_ftrace_events")))

So to avoid this problem I changed to $rawline.

Is there any other alternative available perhaps?
Or should I change back to $rawline?

> []
>
> > +                     if (scalar @conv_array > 0 && $conv_possible == 1) {
> > +                             my $replace = join(' ', @conv_array);
> > +                             if (WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
> > +                                      "$replace is preferred over __attribute__(($attr))\n" . $herecurr) &&
> > +                                     $fix) {
> > +                                     $fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*\Q$attr\E\s*\)\s*\)/$replace/;
>
> I looks it would be useful to add
>                                         $fixed[$fixlinenr] =~ s/\}\Q$replace\E/} $replace/;
> so there's a space added between } and any replacements.
>
Yes I will do that sure.

Thanks,
Dwaipayan.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH v5] checkpatch: extend attributes check to handle more patterns

[Joe Perches]

On Sun, 2020-10-25 at 23:40 +0530, Dwaipayan Ray wrote:
> On Sun, Oct 25, 2020 at 11:29 PM Joe Perches <joe@perches.com> wrote:
[]
> > Using $rawline would also change comments and that seems wrong.
> > Any reason to use $rawline instead of $line?
> Yes I used $line initially but changed it because quoted
> strings were being replaced.
> 
> Like:
> __attribute__((__section__("_ftrace_events")))

Right thanks, that's a very sensible tradeoff to reduce code complexity.

Re: [Linux-kernel-mentees] [PATCH v5] checkpatch: extend attributes check to handle more patterns

[Joe Perches]

On Sun, 2020-10-25 at 23:40 +0530, Dwaipayan Ray wrote:
> On Sun, Oct 25, 2020 at 11:29 PM Joe Perches <joe@perches.com> wrote:
[]
> > Using $rawline would also change comments and that seems wrong.
> > Any reason to use $rawline instead of $line?
> Yes I used $line initially but changed it because quoted
> strings were being replaced.
> 
> Like:
> __attribute__((__section__("_ftrace_events")))

Right thanks, that's a very sensible tradeoff to reduce code complexity.


_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH v5] checkpatch: extend attributes check to handle more patterns

[Dwaipayan Ray]

On Sun, Oct 25, 2020 at 11:49 PM Joe Perches <joe@perches.com> wrote:
>
> On Sun, 2020-10-25 at 23:40 +0530, Dwaipayan Ray wrote:
> > On Sun, Oct 25, 2020 at 11:29 PM Joe Perches <joe@perches.com> wrote:
> []
> > > Using $rawline would also change comments and that seems wrong.
> > > Any reason to use $rawline instead of $line?
> > Yes I used $line initially but changed it because quoted
> > strings were being replaced.
> >
> > Like:
> > __attribute__((__section__("_ftrace_events")))
>
> Right thanks, that's a very sensible tradeoff to reduce code complexity.
>

Okay. Do you see anything else that I should cover in the next
iteration?

If there is no other problem, I will do the space addition one and
resend it to you.

Thanks,
Dwaipayan.

Re: [Linux-kernel-mentees] [PATCH v5] checkpatch: extend attributes check to handle more patterns

[Dwaipayan Ray]

On Sun, Oct 25, 2020 at 11:49 PM Joe Perches <joe@perches.com> wrote:
>
> On Sun, 2020-10-25 at 23:40 +0530, Dwaipayan Ray wrote:
> > On Sun, Oct 25, 2020 at 11:29 PM Joe Perches <joe@perches.com> wrote:
> []
> > > Using $rawline would also change comments and that seems wrong.
> > > Any reason to use $rawline instead of $line?
> > Yes I used $line initially but changed it because quoted
> > strings were being replaced.
> >
> > Like:
> > __attribute__((__section__("_ftrace_events")))
>
> Right thanks, that's a very sensible tradeoff to reduce code complexity.
>

Okay. Do you see anything else that I should cover in the next
iteration?

If there is no other problem, I will do the space addition one and
resend it to you.

Thanks,
Dwaipayan.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH v5] checkpatch: extend attributes check to handle more patterns

[Dwaipayan Ray]

> > +                     if (scalar @conv_array > 0 && $conv_possible == 1) {
> > +                             my $replace = join(' ', @conv_array);
> > +                             if (WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
> > +                                      "$replace is preferred over __attribute__(($attr))\n" . $herecurr) &&
> > +                                     $fix) {
> > +                                     $fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*\Q$attr\E\s*\)\s*\)/$replace/;
>
> I looks it would be useful to add
>                                         $fixed[$fixlinenr] =~ s/\}\Q$replace\E/} $replace/;
> so there's a space added between } and any replacements.
>
Hi once again,
Sorry to interrupt your work so many times.

I tried this and seems the space is being applied even without this
fix.
I think the spacing check applies this fix already.

if (ERROR("SPACING",
"space required after that close brace '}'\n" . $herecurr) &&
$fix) {
$fixed[$fixlinenr] =~
s/}((?!(?:,|;|\)))\S)/} $1/;
}

So do you think it's okay without that perhaps?

Thanks,
Dwaipayan.

Re: [Linux-kernel-mentees] [PATCH v5] checkpatch: extend attributes check to handle more patterns

[Dwaipayan Ray]

> > +                     if (scalar @conv_array > 0 && $conv_possible == 1) {
> > +                             my $replace = join(' ', @conv_array);
> > +                             if (WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
> > +                                      "$replace is preferred over __attribute__(($attr))\n" . $herecurr) &&
> > +                                     $fix) {
> > +                                     $fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*\Q$attr\E\s*\)\s*\)/$replace/;
>
> I looks it would be useful to add
>                                         $fixed[$fixlinenr] =~ s/\}\Q$replace\E/} $replace/;
> so there's a space added between } and any replacements.
>
Hi once again,
Sorry to interrupt your work so many times.

I tried this and seems the space is being applied even without this
fix.
I think the spacing check applies this fix already.

if (ERROR("SPACING",
"space required after that close brace '}'\n" . $herecurr) &&
$fix) {
$fixed[$fixlinenr] =~
s/}((?!(?:,|;|\)))\S)/} $1/;
}

So do you think it's okay without that perhaps?

Thanks,
Dwaipayan.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees