[PATCH 1/1] checkpatch: don't fake typedefs with #define

[PATCH 1/1] checkpatch: don't fake typedefs with #define

[Phil Carmody]

People seemed to be taking the "no typedefs" rule too literally,
and were using #define to abide by the letter rather than the
spirit of the law. E.g. #define FOO_t struct _FOO_t

Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
---
 scripts/checkpatch.pl |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index faea0ec..7dc41c5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2299,6 +2299,12 @@ sub process {
 			     "do not add new typedefs\n" . $herecurr);
 		}
 
+# check for deliberate avoidance of the above anti-typedef rule
+		if ($line =~ /#\s*define\s+$Ident\s+$Type\b/) {
+			WARN("NEW_TYPEDEFS",
+			     "do not fake typedefs using #define\n" . $herecurr);
+		}
+
 # * goes on variable not on type
 		# (char*[ const])
 		while ($line =~ m{(\($NonptrType(\s*(?:$Modifier\b\s*|\*\s*)+)\))}g) {
-- 
1.7.2.5

Re: [PATCH 1/1] checkpatch: don't fake typedefs with #define

[Joe Perches]

On Thu, 2012-05-17 at 15:52 +0300, Phil Carmody wrote:
> People seemed to be taking the "no typedefs" rule too literally,
> and were using #define to abide by the letter rather than the
> spirit of the law. E.g. #define FOO_t struct _FOO_t
> 
> Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
> ---
>  scripts/checkpatch.pl |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index faea0ec..7dc41c5 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2299,6 +2299,12 @@ sub process {
>  			     "do not add new typedefs\n" . $herecurr);
>  		}
>  
> +# check for deliberate avoidance of the above anti-typedef rule
> +		if ($line =~ /#\s*define\s+$Ident\s+$Type\b/) {
> +			WARN("NEW_TYPEDEFS",
> +			     "do not fake typedefs using #define\n" . $herecurr);
> +		}
> +

I think the false positive rate is pretty high.
I used this and don't see too many I'd remove.

$ git grep -E "#\s*define\s+\w+\s+(struct|unsigned|char|short|int|long|const)\b"

Got an example you want this to find?

Re: [PATCH 1/1] checkpatch: don't fake typedefs with #define

[Phil Carmody]

On 17/05/12 13:54 -0700, ext Joe Perches wrote:
> > +# check for deliberate avoidance of the above anti-typedef rule
> > +		if ($line =~ /#\s*define\s+$Ident\s+$Type\b/) {
> > +			WARN("NEW_TYPEDEFS",
> > +			     "do not fake typedefs using #define\n" . $herecurr);
> > +		}
> > +
> 
> I think the false positive rate is pretty high.
> I used this and don't see too many I'd remove.
> 
> $ git grep -E "#\s*define\s+\w+\s+(struct|unsigned|char|short|int|long|const)\b"
> 
> Got an example you want this to find?

Too many. Alas I can't share them. However, your input is most welcome, and that
grep shows that my regexp is *way* too broad. So auto-NACK-ing, and I'll now work
on something that doesn't include most of the above hits. Suggestions for a regexp
banning the fake typedefs that would get flagged were they to be mapped onto a 
typedef would be most welcome. The context I'm seeing them is a clear checkpatch
avoidance mess, and I'd like to see it similarly flagged.

Cheers,
Phil
-- 
Phil Carmody
Tel: +372 5697 1161 

Re: [PATCH 1/1] checkpatch: don't fake typedefs with #define

[Joe Perches]

On Fri, 2012-05-18 at 00:16 +0300, Phil Carmody wrote:
> On 17/05/12 13:54 -0700, ext Joe Perches wrote:
> > > +# check for deliberate avoidance of the above anti-typedef rule
> > > +		if ($line =~ /#\s*define\s+$Ident\s+$Type\b/) {
> > > +			WARN("NEW_TYPEDEFS",
> > > +			     "do not fake typedefs using #define\n" . $herecurr);
> > > +		}
> > > +
> > 
> > I think the false positive rate is pretty high.
> > I used this and don't see too many I'd remove.
> > 
> > $ git grep -E "#\s*define\s+\w+\s+(struct|unsigned|char|short|int|long|const)\b"
> > 
> > Got an example you want this to find?
> 
> Too many. Alas I can't share them.

Too bad.

If all the defines end in _t maybe you could use
	if ($line =~ /^\+\s*#\s*define\s+\w+_t\s+$Type\b/) {
etc...

Re: [PATCH 1/1] checkpatch: don't fake typedefs with #define

[Phil Carmody]

On 17/05/12 14:24 -0700, ext Joe Perches wrote:
> On Fri, 2012-05-18 at 00:16 +0300, Phil Carmody wrote:
> > On 17/05/12 13:54 -0700, ext Joe Perches wrote:
> > > > +# check for deliberate avoidance of the above anti-typedef rule
> > > > +		if ($line =~ /#\s*define\s+$Ident\s+$Type\b/) {
> > > > +			WARN("NEW_TYPEDEFS",
> > > > +			     "do not fake typedefs using #define\n" . $herecurr);
> > > > +		}
> > > > +
> > > 
> > > I think the false positive rate is pretty high.
> > > I used this and don't see too many I'd remove.
> > > 
> > > $ git grep -E "#\s*define\s+\w+\s+(struct|unsigned|char|short|int|long|const)\b"

I hadn't actually run that before, but to be honest, I think
few of them have much justification. Or at least, were that
code to be written today by one of the regular contributors,
it probably wouldn't be written with such a #define. A lot
do go directly against the advice in Documentation/CodingStyle.

> > > Got an example you want this to find?
> > 
> > Too many. Alas I can't share them.
> 
> Too bad.
> 
> If all the defines end in _t maybe you could use
> 	if ($line =~ /^\+\s*#\s*define\s+\w+_t\s+$Type\b/) {
> etc...
> 

Well, certainly structs are the majority of the immediate ones I'd
most like to nail, so how about this:

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index faea0ec..0db7f84 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2299,6 +2299,12 @@ sub process {
                             "do not add new typedefs\n" . $herecurr);
                }
 
+# check for deliberate avoidance of the above anti-typedef rule
+               if ($line =~ /#\s*define\s+$Ident\s+(enum|union|struct)\s+$Ident\b/) {
+                       WARN("NEW_TYPEDEFS",
+                            "do not fake typedefs using #define\n" . $herecurr);
+               }
+
 # * goes on variable not on type
                # (char*[ const])
                while ($line =~ m{(\($NonptrType(\s*(?:$Modifier\b\s*|\*\s*)+)\))}g) {


-- 
Phil Carmody
Tel: +372 5697 1161 

Re: [PATCH 1/1] checkpatch: don't fake typedefs with #define

[Joe Perches]

On Mon, 2012-05-21 at 15:05 +0300, Phil Carmody wrote:
> On 17/05/12 14:24 -0700, ext Joe Perches wrote:
> > On Fri, 2012-05-18 at 00:16 +0300, Phil Carmody wrote:
> > > On 17/05/12 13:54 -0700, ext Joe Perches wrote:
> > > > > +# check for deliberate avoidance of the above anti-typedef rule
> > > > > +		if ($line =~ /#\s*define\s+$Ident\s+$Type\b/) {
> > > > > +			WARN("NEW_TYPEDEFS",
> > > > > +			     "do not fake typedefs using #define\n" . $herecurr);
> > > > > +		}
> > > > > +
> > > > 
> > > > I think the false positive rate is pretty high.
> > > > I used this and don't see too many I'd remove.
> > > > 
> > > > $ git grep -E "#\s*define\s+\w+\s+(struct|unsigned|char|short|int|long|const)\b"
> 
> I hadn't actually run that before, but to be honest, I think
> few of them have much justification. Or at least, were that
> code to be written today by one of the regular contributors,
> it probably wouldn't be written with such a #define. A lot
> do go directly against the advice in Documentation/CodingStyle.
> 
> > > > Got an example you want this to find?
> > > 
> > > Too many. Alas I can't share them.
> > 
> > Too bad.
> > 
> > If all the defines end in _t maybe you could use
> > 	if ($line =~ /^\+\s*#\s*define\s+\w+_t\s+$Type\b/) {
> > etc...
> > 
> 
> Well, certainly structs are the majority of the immediate ones I'd
> most like to nail, so how about this:
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index faea0ec..0db7f84 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2299,6 +2299,12 @@ sub process {
>                              "do not add new typedefs\n" . $herecurr);
>                 }
>  
> +# check for deliberate avoidance of the above anti-typedef rule
> +               if ($line =~ /#\s*define\s+$Ident\s+(enum|union|struct)\s+$Ident\b/) {
> +                       WARN("NEW_TYPEDEFS",
> +                            "do not fake typedefs using #define\n" . $herecurr);
> +               }

I believe this would not catch,

#define typedeflike_define_t	\
	struct foo

If it's deliberate, you probably want to.

So maybe you want to move this and use
the $stat tests like the extern or memset
tests do (look around line 3200).

Re: [PATCH 1/1] checkpatch: don't fake typedefs with #define

[Phil Carmody]

On 21/05/12 09:41 -0700, ext Joe Perches wrote:
> On Mon, 2012-05-21 at 15:05 +0300, Phil Carmody wrote:
> > +# check for deliberate avoidance of the above anti-typedef rule
> > +               if ($line =~ /#\s*define\s+$Ident\s+(enum|union|struct)\s+$Ident\b/) {
 
> I believe this would not catch,
> 
> #define typedeflike_define_t	\
> 	struct foo
> 
> If it's deliberate, you probably want to.
> 
> So maybe you want to move this and use
> the $stat tests like the extern or memset
> tests do (look around line 3200).

Thanks for the pointer. This flags everything I'm interested in:

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index faea0ec..408aee0 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3319,6 +3319,13 @@ sub process {
                             "externs should be avoided in .c files\n" .  $herecurr);
                }
 
+# check for deliberate avoidance of the anti-typedef rule
+               if (defined $stat &&
+                   $stat =~ /#\s*define\s+$Ident\s+(enum|union|struct)\s+$Ident\b/) {
+                       WARN("NEW_TYPEDEFS",
+                            "do not fake typedefs using #define\n" . $herecurr);
+               }
+
 # checks for new __setup's
                if ($rawline =~ /\b__setup\("([^"]*)"/) {
                        my $name = $1;

-- 
Phil Carmody
Tel: +372 5697 1161 

Re: [PATCH 1/1] checkpatch: don't fake typedefs with #define

[Joe Perches]

On Tue, 2012-05-22 at 11:01 +0300, Phil Carmody wrote:
> On 21/05/12 09:41 -0700, ext Joe Perches wrote:
> > On Mon, 2012-05-21 at 15:05 +0300, Phil Carmody wrote:
> > > +# check for deliberate avoidance of the above anti-typedef rule
> > > +               if ($line =~ /#\s*define\s+$Ident\s+(enum|union|struct)\s+$Ident\b/) {
>  
> > I believe this would not catch,
> > 
> > #define typedeflike_define_t	\
> > 	struct foo
> > 
> > If it's deliberate, you probably want to.
> > 
> > So maybe you want to move this and use
> > the $stat tests like the extern or memset
> > tests do (look around line 3200).
> 
> Thanks for the pointer. This flags everything I'm interested in:
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index faea0ec..408aee0 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3319,6 +3319,13 @@ sub process {
>                              "externs should be avoided in .c files\n" .  $herecurr);
>                 }
>  
> +# check for deliberate avoidance of the anti-typedef rule
> +               if (defined $stat &&
> +                   $stat =~ /#\s*define\s+$Ident\s+(enum|union|struct)\s+$Ident\b/) {
> +                       WARN("NEW_TYPEDEFS",
> +                            "do not fake typedefs using #define\n" . $herecurr);
> +               }
> +

This doesn't trigger on the example I gave you.

$ cat def_type.c
#define foo struct bar

#define foo					\
	struct bar

#define foo /* baz */				\
	struct bar

#define foo /* baz */				\
						\
	struct /* baz */ bar
$

It seems for macros the $dstat variable needs to be tested.

Try this:

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index faea0ec..fc4df52 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2933,6 +2933,21 @@ sub process {
 				^\"|\"$
 			}x;
 			#print "REST<$rest> dstat<$dstat> ctx<$ctx>\n";
+
+# check for deliberate avoidance of the anti-typedef rule
+			if ($dstat =~ /(enum|union|struct)\s+$Ident\b/) {
+				$ctx =~ s/\n*$//;
+				my $herectx = $here . "\n";
+				my $cnt = statement_rawlines($ctx);
+
+				for (my $n = 0; $n < $cnt; $n++) {
+					$herectx .= raw_line($linenr, $n) . "\n";
+				}
+
+				WARN("NEW_TYPEDEFS",
+				     "do not fake typedefs using #define\n" . $herectx);
+			}
+
 			if ($dstat ne '' &&
 			    $dstat !~ /^(?:$Ident|-?$Constant),$/ &&			# 10, // foo(),
 			    $dstat !~ /^(?:$Ident|-?$Constant);$/ &&			# foo();

Re: [PATCH 1/1] checkpatch: don't fake typedefs with #define

[Ryan Mallon]

On 18/05/12 07:16, Phil Carmody wrote:

> On 17/05/12 13:54 -0700, ext Joe Perches wrote:
>>> +# check for deliberate avoidance of the above anti-typedef rule
>>> +		if ($line =~ /#\s*define\s+$Ident\s+$Type\b/) {
>>> +			WARN("NEW_TYPEDEFS",
>>> +			     "do not fake typedefs using #define\n" . $herecurr);
>>> +		}
>>> +
>>
>> I think the false positive rate is pretty high.
>> I used this and don't see too many I'd remove.
>>
>> $ git grep -E "#\s*define\s+\w+\s+(struct|unsigned|char|short|int|long|const)\b"
>>
>> Got an example you want this to find?
> 
> Too many. Alas I can't share them. 


That sounds like the cases you have seen are in code which is not
public. I don't think I have ever seen code in the kernel, or in
proposed patches which fakes a typedef with a #define.

Is this an issue for public code, or for a private company tree? In the
latter case, the checkpatch addition should go in your private tree,
rather than mainline. It looks like, at least for mainline Linux, you
are trying to solve a non-existent problem.

~Ryan

Re: [PATCH 1/1] checkpatch: don't fake typedefs with #define

[Joe Perches]

On Wed, 2012-05-23 at 11:54 +1000, Ryan Mallon wrote:
> On 18/05/12 07:16, Phil Carmody wrote:
> > Too many. Alas I can't share them. 
> That sounds like the cases you have seen are in code which is not
> public. I don't think I have ever seen code in the kernel, or in
> proposed patches which fakes a typedef with a #define.
> 
> Is this an issue for public code, or for a private company tree? In the
> latter case, the checkpatch addition should go in your private tree,
> rather than mainline. It looks like, at least for mainline Linux, you
> are trying to solve a non-existent problem.

I agree it's pretty rare.

$ git grep -E "#\s*define\s+\w+\s+(struct|union)\b"|wc -l
57

Perhaps rarity shouldn't be enough to exclude the test though.

If Andy or Andrew wants to take it though, it's not a big
maintenance burden.

Re: [PATCH 1/1] checkpatch: don't fake typedefs with #define

[Ryan Mallon]

On 23/05/12 12:02, Joe Perches wrote:

> On Wed, 2012-05-23 at 11:54 +1000, Ryan Mallon wrote:
>> On 18/05/12 07:16, Phil Carmody wrote:
>>> Too many. Alas I can't share them. 
>> That sounds like the cases you have seen are in code which is not
>> public. I don't think I have ever seen code in the kernel, or in
>> proposed patches which fakes a typedef with a #define.
>>
>> Is this an issue for public code, or for a private company tree? In the
>> latter case, the checkpatch addition should go in your private tree,
>> rather than mainline. It looks like, at least for mainline Linux, you
>> are trying to solve a non-existent problem.
> 
> I agree it's pretty rare.
> 
> $ git grep -E "#\s*define\s+\w+\s+(struct|union)\b"|wc -l
> 57


Some of those do look a bit broken, or easily replaced by typedefs.
Several of them appear to be completely unused. However some of them
look like they need to be defines. The ones in include/net/netfilter are
defines because of this:

  #define nf_ct_ext_find(ext, id) \
          ((id##_TYPE *)__nf_ct_ext_find((ext), (id)))

This one:

  #define __videocard struct card_info
    __attribute__((section(".videocards")))

I'm guessing is because typedefs don't handle __attribute__.

The YYLTYPE one in scripts/dtc/srcpos.h I think is a requirement of
Yacc/Bison.

The acpi_cache_t looks a bit odd, but it is doing an ifdef acpi_cache_t
test (quick glance - looks like it allows a platform specific
definition), though that could probably be reworked to use a typedef and
a __ACPI_CACHE_T_DEFINED or something.

So there are some legitimate reasons for using #define instead of typedef.

However, my real point was that I don't think the problem is rampant. 57
instances across the kernel is not that many, and I haven't seen a lot
of patches trying to add code with #defines where typedefs should be
used. We could test for lots of things in checkpatch, but it seems more
sensible to check for issues which are likely.

~Ryan

Re: [PATCH 1/1] checkpatch: don't fake typedefs with #define

[Joe Perches]

On Wed, 2012-05-23 at 12:50 +1000, Ryan Mallon wrote:
[]
>   #define __videocard struct card_info
>     __attribute__((section(".videocards")))
> I'm guessing is because typedefs don't handle __attribute__.

Probably not and likely it shouldn't be
either a define or a typedef.

It's just not frequently used enough.

Maybe it could be more like
DEFINE_PCI_DEVICE_TABLE though that's
not pretty either.

> We could test for lots of things in checkpatch, but it seems more
> sensible to check for issues which are likely.

Maybe, but being comprehensive isn't
necessarily bad either.

Look at __setup, all the <foo>_ops tests,
the blackfin tests, etc. in checkpatch.

I don't care much one way or another.

Again, if Andrew or Andy wants to push it
forward, it's fine by me.

cheers, Joe