* [PATCH 0/3] checkpatch: Miscellaneous cleanups
@ 2014-10-15 19:32 Joe Perches
2014-10-15 19:32 ` [PATCH 1/3] checkpatch: Add an error test for no space before comma Joe Perches
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Joe Perches @ 2014-10-15 19:32 UTC (permalink / raw)
To: Andrew Morton; +Cc: Andy Whitcroft, linux-kernel
Joe Perches (3):
checkpatch: Add an error test for no space before comma
checkpatch: Add error on use of attribute((weak)) or __weak
checkpatch: Improve test for no space after cast
scripts/checkpatch.pl | 34 +++++++++++++++++++++++++++++++---
1 file changed, 31 insertions(+), 3 deletions(-)
--
2.1.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/3] checkpatch: Add an error test for no space before comma
2014-10-15 19:32 [PATCH 0/3] checkpatch: Miscellaneous cleanups Joe Perches
@ 2014-10-15 19:32 ` Joe Perches
2014-10-15 19:32 ` [PATCH 2/3] checkpatch: Add error on use of attribute((weak)) or __weak Joe Perches
2014-10-15 19:32 ` [PATCH 3/3] checkpatch: Improve test for no space after cast Joe Perches
2 siblings, 0 replies; 10+ messages in thread
From: Joe Perches @ 2014-10-15 19:32 UTC (permalink / raw)
To: Andrew Morton, Andy Whitcroft; +Cc: linux-kernel
Using code like:
int foo , bar;
is not preferred to:
int foo, bar;
so emit an error on this style.
Signed-off-by: Joe Perches <joe@perches.com>
---
scripts/checkpatch.pl | 23 +++++++++++++++++++++--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 374abf4..696254e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3563,14 +3563,33 @@ sub process {
}
}
- # , must have a space on the right.
+ # , must not have a space before and must have a space on the right.
} elsif ($op eq ',') {
+ my $rtrim_before = 0;
+ my $space_after = 0;
+ if ($ctx =~ /Wx./) {
+ if (ERROR("SPACING",
+ "space prohibited before that '$op' $at\n" . $hereptr)) {
+ $line_fixed = 1;
+ $rtrim_before = 1;
+ }
+ }
if ($ctx !~ /.x[WEC]/ && $cc !~ /^}/) {
if (ERROR("SPACING",
"space required after that '$op' $at\n" . $hereptr)) {
- $good = $fix_elements[$n] . trim($fix_elements[$n + 1]) . " ";
$line_fixed = 1;
$last_after = $n;
+ $space_after = 1;
+ }
+ }
+ if ($rtrim_before || $space_after) {
+ if ($rtrim_before) {
+ $good = rtrim($fix_elements[$n]) . trim($fix_elements[$n + 1]);
+ } else {
+ $good = $fix_elements[$n] . trim($fix_elements[$n + 1]);
+ }
+ if ($space_after) {
+ $good .= " ";
}
}
--
2.1.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] checkpatch: Add error on use of attribute((weak)) or __weak
2014-10-15 19:32 [PATCH 0/3] checkpatch: Miscellaneous cleanups Joe Perches
2014-10-15 19:32 ` [PATCH 1/3] checkpatch: Add an error test for no space before comma Joe Perches
@ 2014-10-15 19:32 ` Joe Perches
2014-10-15 19:42 ` Andrew Morton
2014-10-15 19:32 ` [PATCH 3/3] checkpatch: Improve test for no space after cast Joe Perches
2 siblings, 1 reply; 10+ messages in thread
From: Joe Perches @ 2014-10-15 19:32 UTC (permalink / raw)
To: Andrew Morton, Andy Whitcroft; +Cc: linux-kernel
Using weak can have unintended link defects.
Emit an error on its use.
Signed-off-by: Joe Perches <joe@perches.com>
---
scripts/checkpatch.pl | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 696254e..692fdb6 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4671,6 +4671,14 @@ sub process {
}
}
+# Check for __attribute__ weak, or __weak (may have link issues)
+ if ($realfile !~ m@\binclude/uapi/@ &&
+ ($line =~ /\b__attribute__\s*\(\s*\(.*\bweak\b/ ||
+ $line =~ \b__weak\b/)) {
+ ERROR("AVOID_WEAK",
+ "Using weak can have unintended link defects" . $herecurr);
+ }
+
# check for sizeof(&)
if ($line =~ /\bsizeof\s*\(\s*\&/) {
WARN("SIZEOF_ADDRESS",
--
2.1.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] checkpatch: Improve test for no space after cast
2014-10-15 19:32 [PATCH 0/3] checkpatch: Miscellaneous cleanups Joe Perches
2014-10-15 19:32 ` [PATCH 1/3] checkpatch: Add an error test for no space before comma Joe Perches
2014-10-15 19:32 ` [PATCH 2/3] checkpatch: Add error on use of attribute((weak)) or __weak Joe Perches
@ 2014-10-15 19:32 ` Joe Perches
2 siblings, 0 replies; 10+ messages in thread
From: Joe Perches @ 2014-10-15 19:32 UTC (permalink / raw)
To: Andrew Morton, Andy Whitcroft; +Cc: linux-kernel
sizeof(foo) is not a cast, allow a space after it.
Signed-off-by: Joe Perches <joe@perches.com>
Tested-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
scripts/checkpatch.pl | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 692fdb6..21dac46 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2515,7 +2515,8 @@ sub process {
}
}
- if ($line =~ /^\+.*\(\s*$Type\s*\)[ \t]+(?!$Assignment|$Arithmetic|{)/) {
+ if ($line =~ /^\+.*(\w+\s*)?\(\s*$Type\s*\)[ \t]+(?!$Assignment|$Arithmetic|[,;\({\[\<\>])/ &&
+ (!defined($1) || $1 !~ /sizeof\s*/)) {
if (CHK("SPACING",
"No space is necessary after a cast\n" . $herecurr) &&
$fix) {
--
2.1.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] checkpatch: Add error on use of attribute((weak)) or __weak
2014-10-15 19:32 ` [PATCH 2/3] checkpatch: Add error on use of attribute((weak)) or __weak Joe Perches
@ 2014-10-15 19:42 ` Andrew Morton
2014-10-15 19:45 ` Joe Perches
0 siblings, 1 reply; 10+ messages in thread
From: Andrew Morton @ 2014-10-15 19:42 UTC (permalink / raw)
To: Joe Perches; +Cc: Andy Whitcroft, linux-kernel
On Wed, 15 Oct 2014 12:32:08 -0700 Joe Perches <joe@perches.com> wrote:
> Using weak can have unintended link defects.
> Emit an error on its use.
Well, we don't want a warning about use of __weak in function
definitions. Only in declarations.
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -4671,6 +4671,14 @@ sub process {
> }
> }
>
> +# Check for __attribute__ weak, or __weak (may have link issues)
> + if ($realfile !~ m@\binclude/uapi/@ &&
And this bit maybe is checking for use in a header file, which is not
as good as checking for a declaration but is probably good enough.
However the "uapi" bit is confusing.
Can we have a better changelog please?
> + ($line =~ /\b__attribute__\s*\(\s*\(.*\bweak\b/ ||
> + $line =~ \b__weak\b/)) {
> + ERROR("AVOID_WEAK",
> + "Using weak can have unintended link defects" . $herecurr);
> + }
> +
> # check for sizeof(&)
> if ($line =~ /\bsizeof\s*\(\s*\&/) {
> WARN("SIZEOF_ADDRESS",
> --
> 2.1.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] checkpatch: Add error on use of attribute((weak)) or __weak
2014-10-15 19:42 ` Andrew Morton
@ 2014-10-15 19:45 ` Joe Perches
2014-10-15 19:50 ` Andrew Morton
0 siblings, 1 reply; 10+ messages in thread
From: Joe Perches @ 2014-10-15 19:45 UTC (permalink / raw)
To: Andrew Morton; +Cc: Andy Whitcroft, linux-kernel
On Wed, 2014-10-15 at 12:42 -0700, Andrew Morton wrote:
> On Wed, 15 Oct 2014 12:32:08 -0700 Joe Perches <joe@perches.com> wrote:
>
> > Using weak can have unintended link defects.
> > Emit an error on its use.
>
> Well, we don't want a warning about use of __weak in function
> definitions. Only in declarations.
Why is that?
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -4671,6 +4671,14 @@ sub process {
> > }
> > }
> >
> > +# Check for __attribute__ weak, or __weak (may have link issues)
> > + if ($realfile !~ m@\binclude/uapi/@ &&
>
> And this bit maybe is checking for use in a header file, which is not
> as good as checking for a declaration but is probably good enough.
> However the "uapi" bit is confusing.
>
> Can we have a better changelog please?
Suggestions?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] checkpatch: Add error on use of attribute((weak)) or __weak
2014-10-15 19:45 ` Joe Perches
@ 2014-10-15 19:50 ` Andrew Morton
2014-10-15 19:52 ` Joe Perches
0 siblings, 1 reply; 10+ messages in thread
From: Andrew Morton @ 2014-10-15 19:50 UTC (permalink / raw)
To: Joe Perches; +Cc: Andy Whitcroft, linux-kernel
On Wed, 15 Oct 2014 12:45:48 -0700 Joe Perches <joe@perches.com> wrote:
> On Wed, 2014-10-15 at 12:42 -0700, Andrew Morton wrote:
> > On Wed, 15 Oct 2014 12:32:08 -0700 Joe Perches <joe@perches.com> wrote:
> >
> > > Using weak can have unintended link defects.
> > > Emit an error on its use.
> >
> > Well, we don't want a warning about use of __weak in function
> > definitions. Only in declarations.
>
> Why is that?
Because the problem we're trying to detect is when __weak is used on a
declaration.
This is OK:
foo.h:
extern int foo(void);
foo.c:
int __weak foo(void)
{
...
}
But this is not OK:
foo.h:
extern __weak int foo(void);
foo.c:
int __weak foo(void)
{
...
}
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] checkpatch: Add error on use of attribute((weak)) or __weak
2014-10-15 19:50 ` Andrew Morton
@ 2014-10-15 19:52 ` Joe Perches
2014-10-15 20:01 ` Andrew Morton
0 siblings, 1 reply; 10+ messages in thread
From: Joe Perches @ 2014-10-15 19:52 UTC (permalink / raw)
To: Andrew Morton; +Cc: Andy Whitcroft, linux-kernel
On Wed, 2014-10-15 at 12:50 -0700, Andrew Morton wrote:
> On Wed, 15 Oct 2014 12:45:48 -0700 Joe Perches <joe@perches.com> wrote:
>
> > On Wed, 2014-10-15 at 12:42 -0700, Andrew Morton wrote:
> > > On Wed, 15 Oct 2014 12:32:08 -0700 Joe Perches <joe@perches.com> wrote:
> > >
> > > > Using weak can have unintended link defects.
> > > > Emit an error on its use.
> > >
> > > Well, we don't want a warning about use of __weak in function
> > > definitions. Only in declarations.
> >
> > Why is that?
>
> Because the problem we're trying to detect is when __weak is used on a
> declaration.
>
> This is OK:
>
> foo.h:
> extern int foo(void);
> foo.c:
> int __weak foo(void)
> {
> ...
> }
>
> But this is not OK:
>
> foo.h:
> extern __weak int foo(void);
> foo.c:
> int __weak foo(void)
> {
> ...
> }
>
And this?
foo.c:
extern __weak int foo(void);
int __weak foo(void)
{
}
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] checkpatch: Add error on use of attribute((weak)) or __weak
2014-10-15 19:52 ` Joe Perches
@ 2014-10-15 20:01 ` Andrew Morton
2014-10-15 20:46 ` [PATCH 2/3 V2] [PATCH] checkpatch: Add error on use of attribute((weak)) or __weak declarations Joe Perches
0 siblings, 1 reply; 10+ messages in thread
From: Andrew Morton @ 2014-10-15 20:01 UTC (permalink / raw)
To: Joe Perches; +Cc: Andy Whitcroft, linux-kernel
On Wed, 15 Oct 2014 12:52:44 -0700 Joe Perches <joe@perches.com> wrote:
> On Wed, 2014-10-15 at 12:50 -0700, Andrew Morton wrote:
> > On Wed, 15 Oct 2014 12:45:48 -0700 Joe Perches <joe@perches.com> wrote:
> >
> > > On Wed, 2014-10-15 at 12:42 -0700, Andrew Morton wrote:
> > > > On Wed, 15 Oct 2014 12:32:08 -0700 Joe Perches <joe@perches.com> wrote:
> > > >
> > > > > Using weak can have unintended link defects.
> > > > > Emit an error on its use.
> > > >
> > > > Well, we don't want a warning about use of __weak in function
> > > > definitions. Only in declarations.
> > >
> > > Why is that?
> >
> > Because the problem we're trying to detect is when __weak is used on a
> > declaration.
> >
> > This is OK:
> >
> > foo.h:
> > extern int foo(void);
> > foo.c:
> > int __weak foo(void)
> > {
> > ...
> > }
> >
> > But this is not OK:
> >
> > foo.h:
> > extern __weak int foo(void);
> > foo.c:
> > int __weak foo(void)
> > {
> > ...
> > }
> >
>
> And this?
>
> foo.c:
>
> extern __weak int foo(void);
>
> int __weak foo(void)
> {
> }
>
That's why I just said "And this bit maybe is checking for use in a
header file, which is not as good as checking for a declaration but is
probably good enough."
I don't think that would trigger the bug anyway. The problem is that
extern __weak int foo(void);
int foo(void)
{
}
unexpectedly and undesirably turns foo() into __weak.
I think it would be sufficient to check for __weak in a declaration.
If that isn't practical then checking for __weak in a .h file should
suffice.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/3 V2] [PATCH] checkpatch: Add error on use of attribute((weak)) or __weak declarations
2014-10-15 20:01 ` Andrew Morton
@ 2014-10-15 20:46 ` Joe Perches
0 siblings, 0 replies; 10+ messages in thread
From: Joe Perches @ 2014-10-15 20:46 UTC (permalink / raw)
To: Andrew Morton; +Cc: Andy Whitcroft, linux-kernel
Using weak declarations can have unintended link defects.
Emit an error on its use.
Signed-off-by: Joe Perches <joe@perches.com>
---
Maybe you prefer this.
It only warns on declarations.
scripts/checkpatch.pl | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 696254e..8a577aa 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4671,6 +4671,15 @@ sub process {
}
}
+# Check for __attribute__ weak, or __weak declarations (may have link issues)
+ if ($^V && $^V ge 5.10.0 &&
+ $line =~ /(?:$Declare|$DeclareMisordered)\s*$Ident\s*$balanced_parens\s*(?:$Attribute)?\s*;/ &&
+ ($line =~ /\b__attribute__\s*\(\s*\(.*\bweak\b/ ||
+ $line =~ /\b__weak\b/)) {
+ ERROR("WEAK_DECLARATION",
+ "Using weak declarations can have unintended link defects\n" . $herecurr);
+ }
+
# check for sizeof(&)
if ($line =~ /\bsizeof\s*\(\s*\&/) {
WARN("SIZEOF_ADDRESS",
--
2.1.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-10-15 20:46 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-10-15 19:32 [PATCH 0/3] checkpatch: Miscellaneous cleanups Joe Perches
2014-10-15 19:32 ` [PATCH 1/3] checkpatch: Add an error test for no space before comma Joe Perches
2014-10-15 19:32 ` [PATCH 2/3] checkpatch: Add error on use of attribute((weak)) or __weak Joe Perches
2014-10-15 19:42 ` Andrew Morton
2014-10-15 19:45 ` Joe Perches
2014-10-15 19:50 ` Andrew Morton
2014-10-15 19:52 ` Joe Perches
2014-10-15 20:01 ` Andrew Morton
2014-10-15 20:46 ` [PATCH 2/3 V2] [PATCH] checkpatch: Add error on use of attribute((weak)) or __weak declarations Joe Perches
2014-10-15 19:32 ` [PATCH 3/3] checkpatch: Improve test for no space after cast 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).