checkpatch uses various cues in its input files to discover the names of
user-defined types. It then uses that information when processing expressions,
to discover more style issues.
Unfortunately, in rare cases, this means that checkpatch may give different
results if you run it on several files at the same time, or one by one! The
reason is that it may identify a type (or something that looks like a type)
in one file, and then carry this information over when processing a different
file.
As an example, drivers/staging/media/bcm2048/radio-bcm2048.c contains this
line (in a macro):
size value;
Then drivers/staging/media/davinci_vpfe/vpfe_video.c has this line:
while (size * *nbuffers > vpfe_dev->video_limit)
If checkpatch processes these 2 files together, the (spurious) "size" type
detected in the first file will cause it to flag the second file for
improper use of the pointer dereference operator!
Therefore, keep user-defined types in a separate array from built-in ones,
and reset the array of user-defined types at the beginning of each new
source file.
Signed-off-by: Alex Dowad <[email protected]>
---
Dear patch checkers,
I am not a Perl programmer -- please let me know if there is a better way to
accomplish what I am trying to do here.
Your feedback will be appreciated,
Alex Dowad
scripts/checkpatch.pl | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 89b1df4..5a5668f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -424,6 +424,10 @@ our @typeListWithAttr = (
qr{union\s+$InitAttribute\s+$Ident},
);
+# includes user-defined types discovered in the code
+# reset at the beginning of each source file
+our @allTypeList = (@typeList);
+
our @modifierList = (
qr{fastcall},
);
@@ -511,7 +515,7 @@ $misspellings = join("|", sort keys %spelling_fix) if keys %spelling_fix;
sub build_types {
my $mods = "(?x: \n" . join("|\n ", @modifierList) . "\n)";
- my $all = "(?x: \n" . join("|\n ", @typeList) . "\n)";
+ my $all = "(?x: \n" . join("|\n ", @allTypeList) . "\n)";
my $Misordered = "(?x: \n" . join("|\n ", @typeListMisordered) . "\n)";
my $allWithAttr = "(?x: \n" . join("|\n ", @typeListWithAttr) . "\n)";
$Modifier = qr{(?:$Attribute|$Sparse|$mods)};
@@ -745,6 +749,7 @@ for my $filename (@ARGV) {
@fixed = ();
@fixed_inserted = ();
@fixed_deleted = ();
+ @allTypeList = (@typeList);
$fixlinenr = -1;
}
@@ -1616,7 +1621,7 @@ sub possible {
} else {
warn "POSSIBLE: $possible ($line)\n" if ($dbg_possible);
- push(@typeList, $possible);
+ push(@allTypeList, $possible);
}
build_types();
} else {
--
2.0.0.GIT
On Mon, 2015-05-18 at 15:33 +0200, Alex Dowad wrote:
> checkpatch uses various cues in its input files to discover the names of
> user-defined types. It then uses that information when processing expressions,
> to discover more style issues.
>
> Unfortunately, in rare cases, this means that checkpatch may give different
> results if you run it on several files at the same time, or one by one! The
> reason is that it may identify a type (or something that looks like a type)
> in one file, and then carry this information over when processing a different
> file.
True, thanks.
I found the same thing, but hadn't gotten 'round
to submitting a patch for it.
> keep user-defined types in a separate array from built-in ones,
> and reset the array of user-defined types at the beginning of each new
> source file.
[]
> I am not a Perl programmer -- please let me know if there is a better way to
> accomplish what I am trying to do here.
I think nearly no-one _wants_ to be a perl monk.
(Except maybe for the free beer)
> @@ -511,7 +515,7 @@ $misspellings = join("|", sort keys %spelling_fix) if keys %spelling_fix;
>
> sub build_types {
> my $mods = "(?x: \n" . join("|\n ", @modifierList) . "\n)";
> - my $all = "(?x: \n" . join("|\n ", @typeList) . "\n)";
> + my $all = "(?x: \n" . join("|\n ", @allTypeList) . "\n)";
> my $Misordered = "(?x: \n" . join("|\n ", @typeListMisordered) . "\n)";
> my $allWithAttr = "(?x: \n" . join("|\n ", @typeListWithAttr) . "\n)";
> $Modifier = qr{(?:$Attribute|$Sparse|$mods)};
> @@ -745,6 +749,7 @@ for my $filename (@ARGV) {
> @fixed = ();
> @fixed_inserted = ();
> @fixed_deleted = ();
> + @allTypeList = (@typeList);
> $fixlinenr = -1;
> }
I think a proper solution requires a bit more than this.
I believe both @modifierList and @typeList need to be reset
before each new file is processed.
I'll play with it.
Thanks Alex.
On Mon, May 18, 2015 at 03:33:29PM +0200, Alex Dowad wrote:
> checkpatch uses various cues in its input files to discover the names of
> user-defined types. It then uses that information when processing expressions,
> to discover more style issues.
>
> Unfortunately, in rare cases, this means that checkpatch may give different
> results if you run it on several files at the same time, or one by one! The
> reason is that it may identify a type (or something that looks like a type)
> in one file, and then carry this information over when processing a different
> file.
>
> As an example, drivers/staging/media/bcm2048/radio-bcm2048.c contains this
> line (in a macro):
>
> size value;
>
> Then drivers/staging/media/davinci_vpfe/vpfe_video.c has this line:
>
> while (size * *nbuffers > vpfe_dev->video_limit)
>
> If checkpatch processes these 2 files together, the (spurious) "size" type
> detected in the first file will cause it to flag the second file for
> improper use of the pointer dereference operator!
>
> Therefore, keep user-defined types in a separate array from built-in ones,
> and reset the array of user-defined types at the beginning of each new
> source file.
Yes that sounds like confusing behaviour. Resetting the list per file
is probabally more in line with user expectation.
> Signed-off-by: Alex Dowad <[email protected]>
> ---
>
> Dear patch checkers,
>
> I am not a Perl programmer -- please let me know if there is a better way to
> accomplish what I am trying to do here.
>
> Your feedback will be appreciated,
> Alex Dowad
>
> scripts/checkpatch.pl | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 89b1df4..5a5668f 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -424,6 +424,10 @@ our @typeListWithAttr = (
> qr{union\s+$InitAttribute\s+$Ident},
> );
>
> +# includes user-defined types discovered in the code
> +# reset at the beginning of each source file
> +our @allTypeList = (@typeList);
> +
Perhaps @typeListFile or something to say it is per file, then when you
reset it it might be more obvious what you are doing. Or rename the
original list to typeListDefault and then you are making the current one
the default list. Otherwise the change seems to make sense. I did not
check you got all the places which use it.
> our @modifierList = (
> qr{fastcall},
> );
> @@ -511,7 +515,7 @@ $misspellings = join("|", sort keys %spelling_fix) if keys %spelling_fix;
>
> sub build_types {
> my $mods = "(?x: \n" . join("|\n ", @modifierList) . "\n)";
> - my $all = "(?x: \n" . join("|\n ", @typeList) . "\n)";
> + my $all = "(?x: \n" . join("|\n ", @allTypeList) . "\n)";
> my $Misordered = "(?x: \n" . join("|\n ", @typeListMisordered) . "\n)";
> my $allWithAttr = "(?x: \n" . join("|\n ", @typeListWithAttr) . "\n)";
> $Modifier = qr{(?:$Attribute|$Sparse|$mods)};
> @@ -745,6 +749,7 @@ for my $filename (@ARGV) {
> @fixed = ();
> @fixed_inserted = ();
> @fixed_deleted = ();
> + @allTypeList = (@typeList);
> $fixlinenr = -1;
> }
>
> @@ -1616,7 +1621,7 @@ sub possible {
>
> } else {
> warn "POSSIBLE: $possible ($line)\n" if ($dbg_possible);
> - push(@typeList, $possible);
> + push(@allTypeList, $possible);
> }
> build_types();
> } else {
--
> 2.0.0.GIT
-apw
On Mon, 2015-05-18 at 15:33 +0200, Alex Dowad wrote:
> checkpatch uses various cues in its input files to discover the names of
> user-defined types. It then uses that information when processing expressions,
> to discover more style issues.
>
> Unfortunately, in rare cases, this means that checkpatch may give different
> results if you run it on several files at the same time, or one by one! The
> reason is that it may identify a type (or something that looks like a type)
> in one file, and then carry this information over when processing a different
> file.
>
> As an example, drivers/staging/media/bcm2048/radio-bcm2048.c contains this
> line (in a macro):
>
> size value;
>
> Then drivers/staging/media/davinci_vpfe/vpfe_video.c has this line:
>
> while (size * *nbuffers > vpfe_dev->video_limit)
>
> If checkpatch processes these 2 files together, the (spurious) "size" type
> detected in the first file will cause it to flag the second file for
> improper use of the pointer dereference operator!
>
> Therefore, keep user-defined types in a separate array from built-in ones,
> and reset the array of user-defined types at the beginning of each new
> source file.
>
> Signed-off-by: Alex Dowad <[email protected]>
> ---
>
> Dear patch checkers,
>
> I am not a Perl programmer -- please let me know if there is a better way to
> accomplish what I am trying to do here.
>
> Your feedback will be appreciated,
> Alex Dowad
>
> scripts/checkpatch.pl | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
I suggest this:
---
scripts/checkpatch.pl | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 89b1df4..174d711 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -418,6 +418,7 @@ our @typeList = (
qr{${Ident}_handler_fn},
@typeListMisordered,
);
+our @typeListFile = ();
our @typeListWithAttr = (
@typeList,
qr{struct\s+$InitAttribute\s+$Ident},
@@ -427,6 +428,7 @@ our @typeListWithAttr = (
our @modifierList = (
qr{fastcall},
);
+our @modifierListFile = ();
our @mode_permission_funcs = (
["module_param", 3],
@@ -510,8 +512,8 @@ if ($codespell) {
$misspellings = join("|", sort keys %spelling_fix) if keys %spelling_fix;
sub build_types {
- my $mods = "(?x: \n" . join("|\n ", @modifierList) . "\n)";
- my $all = "(?x: \n" . join("|\n ", @typeList) . "\n)";
+ my $mods = "(?x: \n" . join("|\n ", (@modifierList, @modifierListFile)) . "\n)";
+ my $all = "(?x: \n" . join("|\n ", (@typeList, @typeListFile)) . "\n)";
my $Misordered = "(?x: \n" . join("|\n ", @typeListMisordered) . "\n)";
my $allWithAttr = "(?x: \n" . join("|\n ", @typeListWithAttr) . "\n)";
$Modifier = qr{(?:$Attribute|$Sparse|$mods)};
@@ -746,6 +748,9 @@ for my $filename (@ARGV) {
@fixed_inserted = ();
@fixed_deleted = ();
$fixlinenr = -1;
+ @modifierListFile = ();
+ @typeListFile = ();
+ build_types();
}
exit($exit);
@@ -1610,13 +1615,13 @@ sub possible {
for my $modifier (split(' ', $possible)) {
if ($modifier !~ $notPermitted) {
warn "MODIFIER: $modifier ($possible) ($line)\n" if ($dbg_possible);
- push(@modifierList, $modifier);
+ push(@modifierListFile, $modifier);
}
}
} else {
warn "POSSIBLE: $possible ($line)\n" if ($dbg_possible);
- push(@typeList, $possible);
+ push(@typeListFile, $possible);
}
build_types();
} else {
On 18/05/15 20:42, Joe Perches wrote:
> On Mon, 2015-05-18 at 15:33 +0200, Alex Dowad wrote:
>> checkpatch uses various cues in its input files to discover the names of
>> user-defined types. It then uses that information when processing expressions,
>> to discover more style issues.
>>
>> Unfortunately, in rare cases, this means that checkpatch may give different
>> results if you run it on several files at the same time, or one by one! The
>> reason is that it may identify a type (or something that looks like a type)
>> in one file, and then carry this information over when processing a different
>> file.
>>
>> As an example, drivers/staging/media/bcm2048/radio-bcm2048.c contains this
>> line (in a macro):
>>
>> size value;
>>
>> Then drivers/staging/media/davinci_vpfe/vpfe_video.c has this line:
>>
>> while (size * *nbuffers > vpfe_dev->video_limit)
>>
>> If checkpatch processes these 2 files together, the (spurious) "size" type
>> detected in the first file will cause it to flag the second file for
>> improper use of the pointer dereference operator!
>>
>> Therefore, keep user-defined types in a separate array from built-in ones,
>> and reset the array of user-defined types at the beginning of each new
>> source file.
>>
> I suggest this:
> ---
> scripts/checkpatch.pl | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 89b1df4..174d711 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -418,6 +418,7 @@ our @typeList = (
> qr{${Ident}_handler_fn},
> @typeListMisordered,
> );
> +our @typeListFile = ();
> our @typeListWithAttr = (
> @typeList,
> qr{struct\s+$InitAttribute\s+$Ident},
> @@ -427,6 +428,7 @@ our @typeListWithAttr = (
> our @modifierList = (
> qr{fastcall},
> );
> +our @modifierListFile = ();
>
> our @mode_permission_funcs = (
> ["module_param", 3],
> @@ -510,8 +512,8 @@ if ($codespell) {
> $misspellings = join("|", sort keys %spelling_fix) if keys %spelling_fix;
>
> sub build_types {
> - my $mods = "(?x: \n" . join("|\n ", @modifierList) . "\n)";
> - my $all = "(?x: \n" . join("|\n ", @typeList) . "\n)";
> + my $mods = "(?x: \n" . join("|\n ", (@modifierList, @modifierListFile)) . "\n)";
> + my $all = "(?x: \n" . join("|\n ", (@typeList, @typeListFile)) . "\n)";
> my $Misordered = "(?x: \n" . join("|\n ", @typeListMisordered) . "\n)";
> my $allWithAttr = "(?x: \n" . join("|\n ", @typeListWithAttr) . "\n)";
> $Modifier = qr{(?:$Attribute|$Sparse|$mods)};
> @@ -746,6 +748,9 @@ for my $filename (@ARGV) {
> @fixed_inserted = ();
> @fixed_deleted = ();
> $fixlinenr = -1;
> + @modifierListFile = ();
> + @typeListFile = ();
> + build_types();
> }
>
> exit($exit);
> @@ -1610,13 +1615,13 @@ sub possible {
> for my $modifier (split(' ', $possible)) {
> if ($modifier !~ $notPermitted) {
> warn "MODIFIER: $modifier ($possible) ($line)\n" if ($dbg_possible);
> - push(@modifierList, $modifier);
> + push(@modifierListFile, $modifier);
> }
> }
>
> } else {
> warn "POSSIBLE: $possible ($line)\n" if ($dbg_possible);
> - push(@typeList, $possible);
> + push(@typeListFile, $possible);
> }
> build_types();
> } else {
>
>
Looks good! AD
From: Alex Dowad <[email protected]>
checkpatch uses various cues in its input patches and files to discover the
names of user-defined types and modifiers. It then uses that information when
processing expressions to discover potential style issues.
Unfortunately, in rare cases, this means that checkpatch may give different
results if you run it on several input files in one execution, or one by one!
The reason is that it may identify a type (or something that looks like a type)
in one file, and then carry this information over when processing a different
file.
For example, drivers/staging/media/bcm2048/radio-bcm2048.c contains this
line (in a macro):
size value;
and drivers/staging/media/davinci_vpfe/vpfe_video.c has this line:
while (size * *nbuffers > vpfe_dev->video_limit)
If checkpatch processes these 2 files in a single command like:
./scripts/checkpatch.pl -f $file1 $file2
the (spurious) "size" type detected in the first file will cause it to flag
the second file for improper use of the pointer dereference operator.
To fix this, store types and modifiers found in a file in separate arrays
from built-in ones, and reset the arrays of types and modifiers found
in files at the beginning of each new source file.
Signed-off-by: Alex Dowad <[email protected]>
Signed-off-by: Joe Perches <[email protected]>
---
I kept Alex's Signed-off-line as he found the issue, wrote the original
changelog and patch. I did some perl neatening and added the modifier block.
scripts/checkpatch.pl | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 89b1df4..174d711 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -418,6 +418,7 @@ our @typeList = (
qr{${Ident}_handler_fn},
@typeListMisordered,
);
+our @typeListFile = ();
our @typeListWithAttr = (
@typeList,
qr{struct\s+$InitAttribute\s+$Ident},
@@ -427,6 +428,7 @@ our @typeListWithAttr = (
our @modifierList = (
qr{fastcall},
);
+our @modifierListFile = ();
our @mode_permission_funcs = (
["module_param", 3],
@@ -510,8 +512,8 @@ if ($codespell) {
$misspellings = join("|", sort keys %spelling_fix) if keys %spelling_fix;
sub build_types {
- my $mods = "(?x: \n" . join("|\n ", @modifierList) . "\n)";
- my $all = "(?x: \n" . join("|\n ", @typeList) . "\n)";
+ my $mods = "(?x: \n" . join("|\n ", (@modifierList, @modifierListFile)) . "\n)";
+ my $all = "(?x: \n" . join("|\n ", (@typeList, @typeListFile)) . "\n)";
my $Misordered = "(?x: \n" . join("|\n ", @typeListMisordered) . "\n)";
my $allWithAttr = "(?x: \n" . join("|\n ", @typeListWithAttr) . "\n)";
$Modifier = qr{(?:$Attribute|$Sparse|$mods)};
@@ -746,6 +748,9 @@ for my $filename (@ARGV) {
@fixed_inserted = ();
@fixed_deleted = ();
$fixlinenr = -1;
+ @modifierListFile = ();
+ @typeListFile = ();
+ build_types();
}
exit($exit);
@@ -1610,13 +1615,13 @@ sub possible {
for my $modifier (split(' ', $possible)) {
if ($modifier !~ $notPermitted) {
warn "MODIFIER: $modifier ($possible) ($line)\n" if ($dbg_possible);
- push(@modifierList, $modifier);
+ push(@modifierListFile, $modifier);
}
}
} else {
warn "POSSIBLE: $possible ($line)\n" if ($dbg_possible);
- push(@typeList, $possible);
+ push(@typeListFile, $possible);
}
build_types();
} else {
On Mon, May 18, 2015 at 05:22:25PM -0700, Joe Perches wrote:
> From: Alex Dowad <[email protected]>
>
> checkpatch uses various cues in its input patches and files to discover the
> names of user-defined types and modifiers. It then uses that information when
> processing expressions to discover potential style issues.
>
> Unfortunately, in rare cases, this means that checkpatch may give different
> results if you run it on several input files in one execution, or one by one!
>
> The reason is that it may identify a type (or something that looks like a type)
> in one file, and then carry this information over when processing a different
> file.
>
> For example, drivers/staging/media/bcm2048/radio-bcm2048.c contains this
> line (in a macro):
>
> size value;
>
> and drivers/staging/media/davinci_vpfe/vpfe_video.c has this line:
>
> while (size * *nbuffers > vpfe_dev->video_limit)
>
> If checkpatch processes these 2 files in a single command like:
> ./scripts/checkpatch.pl -f $file1 $file2
> the (spurious) "size" type detected in the first file will cause it to flag
> the second file for improper use of the pointer dereference operator.
>
> To fix this, store types and modifiers found in a file in separate arrays
> from built-in ones, and reset the arrays of types and modifiers found
> in files at the beginning of each new source file.
>
> Signed-off-by: Alex Dowad <[email protected]>
> Signed-off-by: Joe Perches <[email protected]>
> ---
>
> I kept Alex's Signed-off-line as he found the issue, wrote the original
> changelog and patch. I did some perl neatening and added the modifier block.
>
> scripts/checkpatch.pl | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 89b1df4..174d711 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -418,6 +418,7 @@ our @typeList = (
> qr{${Ident}_handler_fn},
> @typeListMisordered,
> );
> +our @typeListFile = ();
> our @typeListWithAttr = (
> @typeList,
> qr{struct\s+$InitAttribute\s+$Ident},
> @@ -427,6 +428,7 @@ our @typeListWithAttr = (
> our @modifierList = (
> qr{fastcall},
> );
> +our @modifierListFile = ();
>
> our @mode_permission_funcs = (
> ["module_param", 3],
> @@ -510,8 +512,8 @@ if ($codespell) {
> $misspellings = join("|", sort keys %spelling_fix) if keys %spelling_fix;
>
> sub build_types {
> - my $mods = "(?x: \n" . join("|\n ", @modifierList) . "\n)";
> - my $all = "(?x: \n" . join("|\n ", @typeList) . "\n)";
> + my $mods = "(?x: \n" . join("|\n ", (@modifierList, @modifierListFile)) . "\n)";
> + my $all = "(?x: \n" . join("|\n ", (@typeList, @typeListFile)) . "\n)";
> my $Misordered = "(?x: \n" . join("|\n ", @typeListMisordered) . "\n)";
> my $allWithAttr = "(?x: \n" . join("|\n ", @typeListWithAttr) . "\n)";
> $Modifier = qr{(?:$Attribute|$Sparse|$mods)};
> @@ -746,6 +748,9 @@ for my $filename (@ARGV) {
> @fixed_inserted = ();
> @fixed_deleted = ();
> $fixlinenr = -1;
> + @modifierListFile = ();
> + @typeListFile = ();
> + build_types();
> }
>
> exit($exit);
> @@ -1610,13 +1615,13 @@ sub possible {
> for my $modifier (split(' ', $possible)) {
> if ($modifier !~ $notPermitted) {
> warn "MODIFIER: $modifier ($possible) ($line)\n" if ($dbg_possible);
> - push(@modifierList, $modifier);
> + push(@modifierListFile, $modifier);
> }
> }
>
> } else {
> warn "POSSIBLE: $possible ($line)\n" if ($dbg_possible);
> - push(@typeList, $possible);
> + push(@typeListFile, $possible);
> }
> build_types();
> } else {
That looks about right to me.
Acked-by: Andy Whitcroft <[email protected]>
-apw