All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations
@ 2012-03-22 15:27 Phil Carmody
  2012-03-22 15:49 ` richard -rw- weinberger
  2012-03-22 16:22 ` Jiri Slaby
  0 siblings, 2 replies; 20+ messages in thread
From: Phil Carmody @ 2012-03-22 15:27 UTC (permalink / raw)
  To: apw; +Cc: hpa, ext-phil.2.carmody, linux-kernel

After HPA's wonderful lkml post, referenced, it seems worth trying to
detect this robomatically.

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a3b9782..3993011 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1881,6 +1881,10 @@ sub process {
 				substr($ctx, 0, $name_len + 1, '');
 				$ctx =~ s/\)[^\)]*$//;
 
+				if ($ctx =~ /^\s*(?:\.\.\.)?\s*$/) {
+					# HPA explains why: http://lwn.net/Articles/487493/
+					ERROR("(...) and () are not sufficiently informative function declarations\n$hereline");
+				}
 				for my $arg (split(/\s*,\s*/, $ctx)) {
 					if ($arg =~ /^(?:const\s+)?($Ident)(?:\s+$Sparse)*\s*\**\s*(:?\b$Ident)?$/s || $arg =~ /^($Ident)$/s) {
 
-- 
1.7.2.5


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations
  2012-03-22 15:27 [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations Phil Carmody
@ 2012-03-22 15:49 ` richard -rw- weinberger
  2012-03-22 16:33   ` Joe Perches
  2012-03-22 16:22 ` Jiri Slaby
  1 sibling, 1 reply; 20+ messages in thread
From: richard -rw- weinberger @ 2012-03-22 15:49 UTC (permalink / raw)
  To: Phil Carmody; +Cc: apw, hpa, linux-kernel

On Thu, Mar 22, 2012 at 4:27 PM, Phil Carmody
<ext-phil.2.carmody@nokia.com> wrote:
> After HPA's wonderful lkml post, referenced, it seems worth trying to
> detect this robomatically.

See:
http://marc.info/?l=linux-kernel&m=133193918813599

-- 
Thanks,
//richard

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations
  2012-03-22 15:27 [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations Phil Carmody
  2012-03-22 15:49 ` richard -rw- weinberger
@ 2012-03-22 16:22 ` Jiri Slaby
  2012-03-22 16:49   ` Valdis.Kletnieks
                     ` (4 more replies)
  1 sibling, 5 replies; 20+ messages in thread
From: Jiri Slaby @ 2012-03-22 16:22 UTC (permalink / raw)
  To: Phil Carmody; +Cc: apw, hpa, linux-kernel

On 03/22/2012 04:27 PM, Phil Carmody wrote:
> After HPA's wonderful lkml post, referenced, it seems worth trying to
> detect this robomatically.
> 
> Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
> ---
>  scripts/checkpatch.pl |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index a3b9782..3993011 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1881,6 +1881,10 @@ sub process {
>  				substr($ctx, 0, $name_len + 1, '');
>  				$ctx =~ s/\)[^\)]*$//;
>  
> +				if ($ctx =~ /^\s*(?:\.\.\.)?\s*$/) {
> +					# HPA explains why: http://lwn.net/Articles/487493/
> +					ERROR("(...) and () are not sufficiently informative function declarations\n$hereline");
> +				}

That explanation is not fully correct. C99 explicitly says (6.7.5.3.14):
An identifier list declares only the identifiers of the parameters of
the function. An empty list in a function declarator that is part of a
definition of that function specifies that the function has no
parameters. The empty list in a function declarator that is not part of
a definition of that function specifies that no information about the
number or types of the parameters is supplied.

So what you are trying to force here holds only for (forward)
declarations. Not for functions with definitions (bodies). Is checkpatch
capable to differ between those?

thanks,
-- js suse labs


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations
  2012-03-22 15:49 ` richard -rw- weinberger
@ 2012-03-22 16:33   ` Joe Perches
  0 siblings, 0 replies; 20+ messages in thread
From: Joe Perches @ 2012-03-22 16:33 UTC (permalink / raw)
  To: richard -rw- weinberger; +Cc: Phil Carmody, apw, hpa, linux-kernel

On Thu, 2012-03-22 at 16:49 +0100, richard -rw- weinberger wrote:
> On Thu, Mar 22, 2012 at 4:27 PM, Phil Carmody
> <ext-phil.2.carmody@nokia.com> wrote:
> > After HPA's wonderful lkml post, referenced, it seems worth trying to
> > detect this robomatically.
> 
> See:
> http://marc.info/?l=linux-kernel&m=133193918813599

But Phil's test is better because it also
tests for function declarations on multiple
lines like

type
foo(...)

		if ($ctx =~ /((\b$Type\s+$Ident)\s*\(\s*(?:\.\.\.)?\s*\))/) {

The ERROR needs updating for a --ignore type though.
Something like this may be better.

		ERROR("FUNCTION_NO_PROTOTYPE",
		      "Bad function definition - $1 should probably be $2(void)\n" .
		      herecurr);

And I think the screed funny once but not useful.



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations
  2012-03-22 16:22 ` Jiri Slaby
@ 2012-03-22 16:49   ` Valdis.Kletnieks
  2012-03-22 16:55     ` Jiri Slaby
  2012-03-22 16:53   ` H. Peter Anvin
                     ` (3 subsequent siblings)
  4 siblings, 1 reply; 20+ messages in thread
From: Valdis.Kletnieks @ 2012-03-22 16:49 UTC (permalink / raw)
  To: Jiri Slaby; +Cc: Phil Carmody, apw, hpa, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 984 bytes --]

On Thu, 22 Mar 2012 17:22:33 +0100, Jiri Slaby said:
> That explanation is not fully correct. C99 explicitly says (6.7.5.3.14):
> An identifier list declares only the identifiers of the parameters of
> the function. An empty list in a function declarator that is part of a
> definition of that function specifies that the function has no
> parameters. The empty list in a function declarator that is not part of
> a definition of that function specifies that no information about the
> number or types of the parameters is supplied.
>
> So what you are trying to force here holds only for (forward)
> declarations. Not for functions with definitions (bodies). Is checkpatch
> capable to differ between those?

The fact that 'int foo() { /*whatever*/ }' with an empty parameter list
is *legal* doesn't mean that we can't collectively put our foot down and
say "This is too ugly to live in our source tree".

Is there any *legitimate* use of an empty parameter list in the kernel tree?

[-- Attachment #2: Type: application/pgp-signature, Size: 865 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations
  2012-03-22 16:22 ` Jiri Slaby
  2012-03-22 16:49   ` Valdis.Kletnieks
@ 2012-03-22 16:53   ` H. Peter Anvin
  2012-03-22 16:56     ` Jiri Slaby
  2012-03-22 17:48     ` Phil Carmody
  2012-03-22 17:17   ` Nick Bowler
                     ` (2 subsequent siblings)
  4 siblings, 2 replies; 20+ messages in thread
From: H. Peter Anvin @ 2012-03-22 16:53 UTC (permalink / raw)
  To: Jiri Slaby; +Cc: Phil Carmody, apw, linux-kernel

On 03/22/2012 09:22 AM, Jiri Slaby wrote:
> 
> That explanation is not fully correct. C99 explicitly says (6.7.5.3.14):
> An identifier list declares only the identifiers of the parameters of
> the function. An empty list in a function declarator that is part of a
> definition of that function specifies that the function has no
> parameters. The empty list in a function declarator that is not part of
> a definition of that function specifies that no information about the
> number or types of the parameters is supplied.
> 
> So what you are trying to force here holds only for (forward)
> declarations. Not for functions with definitions (bodies). Is checkpatch
> capable to differ between those?
> 

We shouldn't use it anyway.  gcc might take it that way in C99 mode, but
it's unclear if it does it in the default mode, and having declarators
and definitions be different is just asking for trouble.

	-hpa


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations
  2012-03-22 16:49   ` Valdis.Kletnieks
@ 2012-03-22 16:55     ` Jiri Slaby
  2012-03-22 17:00       ` Jiri Slaby
  2012-03-22 17:17       ` Valdis.Kletnieks
  0 siblings, 2 replies; 20+ messages in thread
From: Jiri Slaby @ 2012-03-22 16:55 UTC (permalink / raw)
  To: Valdis.Kletnieks; +Cc: Phil Carmody, apw, hpa, linux-kernel

On 03/22/2012 05:49 PM, Valdis.Kletnieks@vt.edu wrote:
> On Thu, 22 Mar 2012 17:22:33 +0100, Jiri Slaby said:
>> That explanation is not fully correct. C99 explicitly says
>> (6.7.5.3.14): An identifier list declares only the identifiers of
>> the parameters of the function. An empty list in a function
>> declarator that is part of a definition of that function
>> specifies that the function has no parameters. The empty list in
>> a function declarator that is not part of a definition of that
>> function specifies that no information about the number or types
>> of the parameters is supplied.
>> 
>> So what you are trying to force here holds only for (forward) 
>> declarations. Not for functions with definitions (bodies). Is
>> checkpatch capable to differ between those?
> 
> The fact that 'int foo() { /*whatever*/ }' with an empty parameter
> list is *legal* doesn't mean that we can't collectively put our
> foot down and say "This is too ugly to live in our source tree".

Yes. And I even thought that we have this in CodingStyle, but I didn't
find it.

> Is there any *legitimate* use of an empty parameter list in the
> kernel tree?

Yeah, a ton of them. There are many drivers which work with a global
singleton. So they need no argument.

thanks.
-- js suse labs



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations
  2012-03-22 16:53   ` H. Peter Anvin
@ 2012-03-22 16:56     ` Jiri Slaby
  2012-03-22 17:48     ` Phil Carmody
  1 sibling, 0 replies; 20+ messages in thread
From: Jiri Slaby @ 2012-03-22 16:56 UTC (permalink / raw)
  To: H. Peter Anvin; +Cc: Phil Carmody, apw, linux-kernel

On 03/22/2012 05:53 PM, H. Peter Anvin wrote:
> having declarators
> and definitions be different is just asking for trouble.

Yes, agreed.

-- js suse labs


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations
  2012-03-22 16:55     ` Jiri Slaby
@ 2012-03-22 17:00       ` Jiri Slaby
  2012-03-22 17:17       ` Valdis.Kletnieks
  1 sibling, 0 replies; 20+ messages in thread
From: Jiri Slaby @ 2012-03-22 17:00 UTC (permalink / raw)
  To: Valdis.Kletnieks; +Cc: Phil Carmody, apw, hpa, linux-kernel

On 03/22/2012 05:55 PM, Jiri Slaby wrote:
> On 03/22/2012 05:49 PM, Valdis.Kletnieks@vt.edu wrote:
>> On Thu, 22 Mar 2012 17:22:33 +0100, Jiri Slaby said:
>>> That explanation is not fully correct. C99 explicitly says
>>> (6.7.5.3.14): An identifier list declares only the identifiers of
>>> the parameters of the function. An empty list in a function
>>> declarator that is part of a definition of that function
>>> specifies that the function has no parameters. The empty list in
>>> a function declarator that is not part of a definition of that
>>> function specifies that no information about the number or types
>>> of the parameters is supplied.
>>>
>>> So what you are trying to force here holds only for (forward) 
>>> declarations. Not for functions with definitions (bodies). Is
>>> checkpatch capable to differ between those?
>>
>> The fact that 'int foo() { /*whatever*/ }' with an empty parameter
>> list is *legal* doesn't mean that we can't collectively put our
>> foot down and say "This is too ugly to live in our source tree".

And I pointed that out because I didn't want people to start converting
such uses in batches now.

thanks,
-- js suse labs



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations
  2012-03-22 16:22 ` Jiri Slaby
  2012-03-22 16:49   ` Valdis.Kletnieks
  2012-03-22 16:53   ` H. Peter Anvin
@ 2012-03-22 17:17   ` Nick Bowler
  2012-03-22 17:19     ` Nick Bowler
  2012-03-26 10:03     ` Pedro Alves
  2012-03-22 17:32   ` Phil Carmody
  2012-04-15 18:18   ` Phil Carmody
  4 siblings, 2 replies; 20+ messages in thread
From: Nick Bowler @ 2012-03-22 17:17 UTC (permalink / raw)
  To: Jiri Slaby; +Cc: Phil Carmody, apw, hpa, linux-kernel

On 2012-03-22 17:22 +0100, Jiri Slaby wrote:
> On 03/22/2012 04:27 PM, Phil Carmody wrote:
[...]
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index a3b9782..3993011 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -1881,6 +1881,10 @@ sub process {
> >  				substr($ctx, 0, $name_len + 1, '');
> >  				$ctx =~ s/\)[^\)]*$//;
> >  
> > +				if ($ctx =~ /^\s*(?:\.\.\.)?\s*$/) {
> > +					# HPA explains why: http://lwn.net/Articles/487493/
> > +					ERROR("(...) and () are not sufficiently informative function declarations\n$hereline");
> > +				}
> 
> That explanation is not fully correct. C99 explicitly says (6.7.5.3.14):
> An identifier list declares only the identifiers of the parameters of
> the function. An empty list in a function declarator that is part of a
> definition of that function specifies that the function has no
> parameters.

Nevertheless, an empty identifier list in a declaration is still not the
same as a parameter type list with (void).  In particular, the empty
identifier list *is not a prototype declaration for the function*.  That
means that arguments passed to the function are not subject to the usual
checks/conversions implied by a prototype.

Consider:

  int foo()
  {
     return 0;
  }

  int main(void)
  {
    return foo(1, 2, 3, 4, 5); /* this is syntactically OK; undefined
                                  behaviour at runtime. */
  }

GCC will not normally warn about the above (unless you pass
-Wold-style-definition) which warns for all function definitions that
lack a prototype.  On the other hand, changing it to int foo(void)
provides the required prototype for the arguments to be checked, and the
above becomes a proper error.

> So what you are trying to force here holds only for (forward)
> declarations. Not for functions with definitions (bodies). Is
> checkpatch capable to differ between those?

For the above reasons, non-prototype declarations of any sort should be
avoided.  No need for checkpatch to distinguish between whether or not
there's a function body.

Cheers,
-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations
  2012-03-22 16:55     ` Jiri Slaby
  2012-03-22 17:00       ` Jiri Slaby
@ 2012-03-22 17:17       ` Valdis.Kletnieks
  2012-03-22 19:00         ` Joe Perches
  1 sibling, 1 reply; 20+ messages in thread
From: Valdis.Kletnieks @ 2012-03-22 17:17 UTC (permalink / raw)
  To: Jiri Slaby; +Cc: Phil Carmody, apw, hpa, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 691 bytes --]

On Thu, 22 Mar 2012 17:55:38 +0100, Jiri Slaby said:
> On 03/22/2012 05:49 PM, Valdis.Kletnieks@vt.edu wrote:
> > Is there any *legitimate* use of an empty parameter list in the
> > kernel tree?
>
> Yeah, a ton of them. There are many drivers which work with a global
> singleton. So they need no argument.

Those can be 'int foo(void)' can't they?  The other historical usage is for
'int foo()' to denote an old K&R-style varargs list, which is like disco - a
bad idea from long agon that's never coming back. ;)

Of course, if we add this to checkpatch, we'll have a flood of fixup patches.
Maybe we just need to say "3.5 will be the int foo() housecleaning release" and
be done with it?


[-- Attachment #2: Type: application/pgp-signature, Size: 865 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations
  2012-03-22 17:17   ` Nick Bowler
@ 2012-03-22 17:19     ` Nick Bowler
  2012-03-26 10:03     ` Pedro Alves
  1 sibling, 0 replies; 20+ messages in thread
From: Nick Bowler @ 2012-03-22 17:19 UTC (permalink / raw)
  To: Jiri Slaby; +Cc: Phil Carmody, apw, hpa, linux-kernel

On 2012-03-22 13:17 -0400, Nick Bowler wrote:
> On 2012-03-22 17:22 +0100, Jiri Slaby wrote:
> > On 03/22/2012 04:27 PM, Phil Carmody wrote:
> [...]
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > index a3b9782..3993011 100755
> > > --- a/scripts/checkpatch.pl
> > > +++ b/scripts/checkpatch.pl
> > > @@ -1881,6 +1881,10 @@ sub process {
> > >  				substr($ctx, 0, $name_len + 1, '');
> > >  				$ctx =~ s/\)[^\)]*$//;
> > >  
> > > +				if ($ctx =~ /^\s*(?:\.\.\.)?\s*$/) {
> > > +					# HPA explains why: http://lwn.net/Articles/487493/
> > > +					ERROR("(...) and () are not sufficiently informative function declarations\n$hereline");
> > > +				}
> > 
> > That explanation is not fully correct. C99 explicitly says (6.7.5.3.14):
> > An identifier list declares only the identifiers of the parameters of
> > the function. An empty list in a function declarator that is part of a
> > definition of that function specifies that the function has no
> > parameters.
> 
> Nevertheless, an empty identifier list in a declaration is still not the
                                              ^^^^^^^^^^^
That should obviously have said "definition".  Sigh.

> same as a parameter type list with (void).  In particular, the empty
> identifier list *is not a prototype declaration for the function*.  That
> means that arguments passed to the function are not subject to the usual
> checks/conversions implied by a prototype.
> 
> Consider:
> 
>   int foo()
>   {
>      return 0;
>   }
> 
>   int main(void)
>   {
>     return foo(1, 2, 3, 4, 5); /* this is syntactically OK; undefined
>                                   behaviour at runtime. */
>   }
> 
> GCC will not normally warn about the above (unless you pass
> -Wold-style-definition) which warns for all function definitions that
> lack a prototype.  On the other hand, changing it to int foo(void)
> provides the required prototype for the arguments to be checked, and the
> above becomes a proper error.
> 
> > So what you are trying to force here holds only for (forward)
> > declarations. Not for functions with definitions (bodies). Is
> > checkpatch capable to differ between those?
> 
> For the above reasons, non-prototype declarations of any sort should be
> avoided.  No need for checkpatch to distinguish between whether or not
> there's a function body.
> 
> Cheers,
-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations
  2012-03-22 16:22 ` Jiri Slaby
                     ` (2 preceding siblings ...)
  2012-03-22 17:17   ` Nick Bowler
@ 2012-03-22 17:32   ` Phil Carmody
  2012-04-15 18:18   ` Phil Carmody
  4 siblings, 0 replies; 20+ messages in thread
From: Phil Carmody @ 2012-03-22 17:32 UTC (permalink / raw)
  To: ext Jiri Slaby; +Cc: apw, hpa, linux-kernel

On 22/03/12 17:22 +0100, ext Jiri Slaby wrote:
> On 03/22/2012 04:27 PM, Phil Carmody wrote:
> > After HPA's wonderful lkml post, referenced, it seems worth trying to
> > detect this robomatically.
> > 
> > Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
> > ---
> >  scripts/checkpatch.pl |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index a3b9782..3993011 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -1881,6 +1881,10 @@ sub process {
> >  				substr($ctx, 0, $name_len + 1, '');
> >  				$ctx =~ s/\)[^\)]*$//;
> >  
> > +				if ($ctx =~ /^\s*(?:\.\.\.)?\s*$/) {
> > +					# HPA explains why: http://lwn.net/Articles/487493/
> > +					ERROR("(...) and () are not sufficiently informative function declarations\n$hereline");
> > +				}
> 
> That explanation is not fully correct. C99 explicitly says (6.7.5.3.14):
> An identifier list declares only the identifiers of the parameters of
> the function. An empty list in a function declarator that is part of a
> definition of that function specifies that the function has no
> parameters. The empty list in a function declarator that is not part of
> a definition of that function specifies that no information about the
> number or types of the parameters is supplied.
>
> So what you are trying to force here holds only for (forward)
> declarations. Not for functions with definitions (bodies). Is checkpatch
> capable to differ between those?

Damn good catch. I will admit that my first attempt was practically
identical to RW's (which I hadn't seen, LKML moves too quickly for me
to follow), but when I searched around for 'declaration' I came across
the above location, and it seemed more appropriate. Does the earlier
patch have the same issue? I presume it does given that the comment is
"""
Functions like this one are evil:

void foo()
{
	...
}
"""

I did a quick grep of the code, and there are few instances of
definitions with no parameters compared to the number with (void). It
might be bold, but could one say that from a _style_ (rather than
correctness) perspective, migrating everyone towards (void) everywhere
is a good thing?

However, if not, all is not lost - even if we don't know the full 
context, the existence of a semicolon after the () might be enough
to be useful.

I can re-bodge with a semicolon if noone offers more info about 
checkpatch.pl's knowledge of context, and uniformity of style is
not considered positive in these situations.

Thanks all for your input,
Phil

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations
  2012-03-22 16:53   ` H. Peter Anvin
  2012-03-22 16:56     ` Jiri Slaby
@ 2012-03-22 17:48     ` Phil Carmody
  2012-03-22 19:10       ` Peter Seebach
  1 sibling, 1 reply; 20+ messages in thread
From: Phil Carmody @ 2012-03-22 17:48 UTC (permalink / raw)
  To: ext H. Peter Anvin; +Cc: Jiri Slaby, apw, linux-kernel

On 22/03/12 09:53 -0700, ext H. Peter Anvin wrote:
> On 03/22/2012 09:22 AM, Jiri Slaby wrote:
> > 
> > That explanation is not fully correct. C99 explicitly says (6.7.5.3.14):
> > An identifier list declares only the identifiers of the parameters of
> > the function. An empty list in a function declarator that is part of a
> > definition of that function specifies that the function has no
> > parameters. The empty list in a function declarator that is not part of
> > a definition of that function specifies that no information about the
> > number or types of the parameters is supplied.
> > 
> > So what you are trying to force here holds only for (forward)
> > declarations. Not for functions with definitions (bodies). Is checkpatch
> > capable to differ between those?
> 
> We shouldn't use it anyway.  gcc might take it that way in C99 mode, but
> it's unclear if it does it in the default mode, and having declarators
> and definitions be different is just asking for trouble.

This perhaps? (And my support goes to any better rewordings.)


From: Phil Carmody <ext-phil.2.carmody@nokia.com>
Date: Thu, 22 Mar 2012 19:40:47 +0200
Subject: [PATCH 2/2] Documentation/CodingStyle: outlaw () use (void)

While empty paramter lists in function definitions are not technically
wrong, those situations are rare enough that it's worth encouraging
people towards a more uniform, always unambiguous, style.
http://lkml.org/lkml/2012/3/22/241

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

diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
index 2b90d32..c4b9df7 100644
--- a/Documentation/CodingStyle
+++ b/Documentation/CodingStyle
@@ -381,6 +381,8 @@ EXPORT_SYMBOL(system_is_up);
 In function prototypes, include parameter names with their data types.
 Although this is not required by the C language, it is preferred in Linux
 because it is a simple way to add valuable information for the reader.
+Do not use empty parameter lists, as they sometimes mean one thing and
+sometimes mean another - avoid ambiguity by using (void).
 
 
 		Chapter 7: Centralized exiting of functions
-- 
1.7.2.5

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations
  2012-03-22 17:17       ` Valdis.Kletnieks
@ 2012-03-22 19:00         ` Joe Perches
  0 siblings, 0 replies; 20+ messages in thread
From: Joe Perches @ 2012-03-22 19:00 UTC (permalink / raw)
  To: Valdis.Kletnieks; +Cc: Jiri Slaby, Phil Carmody, apw, hpa, linux-kernel

On Thu, 2012-03-22 at 13:17 -0400, Valdis.Kletnieks@vt.edu wrote:
> Those can be 'int foo(void)' can't they?  The other historical usage is for
> 'int foo()' to denote an old K&R-style varargs list, which is like disco - a
> bad idea from long agon that's never coming back. ;)
> 
> Of course, if we add this to checkpatch, we'll have a flood of fixup patches.

Not really, more like a mist than a flood.
There just aren't many in-kernel uses.
There are 10 in drivers/, 15 in arch/, none in include/

$ grep -rP --include=*.[ch] "^\w+\s+\w+\s*\(\s*(?:\.\.\.){0,1}\s*\)" *

A single trivial patch should work for all of them.

> Maybe we just need to say "3.5 will be the int foo() housecleaning release" and
> be done with it?

Sensible.


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations
  2012-03-22 17:48     ` Phil Carmody
@ 2012-03-22 19:10       ` Peter Seebach
  2012-03-22 20:01         ` Phil Carmody
  0 siblings, 1 reply; 20+ messages in thread
From: Peter Seebach @ 2012-03-22 19:10 UTC (permalink / raw)
  To: Phil Carmody; +Cc: ext H. Peter Anvin, Jiri Slaby, apw, linux-kernel

On Thu, 22 Mar 2012 19:48:02 +0200
Phil Carmody <ext-phil.2.carmody@nokia.com> wrote:

> While empty paramter lists in function definitions are not technically
> wrong, those situations are rare enough that it's worth encouraging
> people towards a more uniform, always unambiguous, style.

Typo here ("paramter").

You don't need to check for (...).  That doesn't actually exist, and
gcc rejects it (as it should).  The description of () as meaning (...)
is a (slight) oversimplification.  As to the () case:

The rules here are complicated.  Complicated enough that I don't even
TRY to remember them.  Fundamentally, f() is equivalent to either
f(void) or f(please do unpleasant things to me with railroad spikes).
I would definitely endorse a policy discouraging its use.

I can only think of one exception, and it's inapplicable.  The
exception:  Imagine that you wish to write a wrapper for a function
like scandir, which takes a function pointer as an argument, and you
wish the wrapper to work on two systems where the arguments passed to
the function pointer are of different types, but you yourself will
never actually see or care about the arguments; you just want to pass
the function pointer around.  Then it could make sense to declare the
argument as int (*compar)().  But that's a once-a-lifetime use case,
give or take.

-s
-- 
Listen, get this.  Nobody with a good compiler needs to be justified.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations
  2012-03-22 19:10       ` Peter Seebach
@ 2012-03-22 20:01         ` Phil Carmody
  0 siblings, 0 replies; 20+ messages in thread
From: Phil Carmody @ 2012-03-22 20:01 UTC (permalink / raw)
  To: ext Peter Seebach; +Cc: ext H. Peter Anvin, Jiri Slaby, apw, linux-kernel

On 22/03/12 14:10 -0500, ext Peter Seebach wrote:
> On Thu, 22 Mar 2012 19:48:02 +0200
> Phil Carmody <ext-phil.2.carmody@nokia.com> wrote:
> 
> > While empty paramter lists in function definitions are not technically
> > wrong, those situations are rare enough that it's worth encouraging
> > people towards a more uniform, always unambiguous, style.
> 
> Typo here ("paramter").
> 
> You don't need to check for (...).  That doesn't actually exist, and
> gcc rejects it (as it should).  The description of () as meaning (...)
> is a (slight) oversimplification.

Thanks, and thanks, yup. I just got carried away by HPA's wonderful tale, 
I took it too literally.

>  As to the () case:
> 
> The rules here are complicated.  Complicated enough that I don't even
> TRY to remember them.  Fundamentally, f() is equivalent to either
> f(void) or f(please do unpleasant things to me with railroad spikes).
> I would definitely endorse a policy discouraging its use.

It appears we have quite broad agreement, it's just the details that
need to be polished.

> I can only think of one exception, and it's inapplicable.  The
> exception:  Imagine that you wish to write a wrapper for a function
> like scandir, which takes a function pointer as an argument, and you
> wish the wrapper to work on two systems where the arguments passed to
> the function pointer are of different types, but you yourself will
> never actually see or care about the arguments; you just want to pass
> the function pointer around.  Then it could make sense to declare the
> argument as int (*compar)().  But that's a once-a-lifetime use case,
> give or take.

Maybe WG14 should introduce (...) for just that case! :-D

Cheers for your input Seebs,
Phil

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations
  2012-03-22 17:17   ` Nick Bowler
  2012-03-22 17:19     ` Nick Bowler
@ 2012-03-26 10:03     ` Pedro Alves
  2012-04-16  6:11       ` H. Peter Anvin
  1 sibling, 1 reply; 20+ messages in thread
From: Pedro Alves @ 2012-03-26 10:03 UTC (permalink / raw)
  To: Nick Bowler; +Cc: Jiri Slaby, Phil Carmody, apw, hpa, linux-kernel

On 03/22/2012 05:17 PM, Nick Bowler wrote:

> GCC will not normally warn about the above (unless you pass
> -Wold-style-definition) which warns for all function definitions that
> lack a prototype.


/me asks the stupid question:

Why not make the compiler catch this instead then, with -Werror=old-style-definition?

-- 
Pedro Alves

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations
  2012-03-22 16:22 ` Jiri Slaby
                     ` (3 preceding siblings ...)
  2012-03-22 17:32   ` Phil Carmody
@ 2012-04-15 18:18   ` Phil Carmody
  4 siblings, 0 replies; 20+ messages in thread
From: Phil Carmody @ 2012-04-15 18:18 UTC (permalink / raw)
  To: ext Jiri Slaby; +Cc: apw, hpa, linux-kernel

On 22/03/12 17:22 +0100, ext Jiri Slaby wrote:
> On 03/22/2012 04:27 PM, Phil Carmody wrote:
> > After HPA's wonderful lkml post, referenced, it seems worth trying to
> > detect this robomatically.
> > 
> > Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
> > ---
> >  scripts/checkpatch.pl |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index a3b9782..3993011 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -1881,6 +1881,10 @@ sub process {
> >  				substr($ctx, 0, $name_len + 1, '');
> >  				$ctx =~ s/\)[^\)]*$//;
> >  
> > +				if ($ctx =~ /^\s*(?:\.\.\.)?\s*$/) {
> > +					# HPA explains why: http://lwn.net/Articles/487493/
> > +					ERROR("(...) and () are not sufficiently informative function declarations\n$hereline");
> > +				}
> 
> That explanation is not fully correct. C99 explicitly says (6.7.5.3.14):
> An identifier list declares only the identifiers of the parameters of
> the function. An empty list in a function declarator that is part of a
> definition of that function specifies that the function has no
> parameters. The empty list in a function declarator that is not part of
> a definition of that function specifies that no information about the
> number or types of the parameters is supplied.
> 
> So what you are trying to force here holds only for (forward)
> declarations. Not for functions with definitions (bodies). Is checkpatch
> capable to differ between those?

I know I've already agreed to the above, as it makes perfect sense, but 
I've just come across this, and it appears we're both wrong.

http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_317.htm
""""
...
void f(){}
...
Question 1: Does such a function definition give the function a type including a prototype for the rest of the translation unit?
...
Committee Response
The grammar states that an empty parens stands for an empty identifier list not an empty parameter-type-list.
The answer to question #1 is NO
"""

So it appears () is never sufficiently informative.
Phil
-- 
Phil Carmody

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations
  2012-03-26 10:03     ` Pedro Alves
@ 2012-04-16  6:11       ` H. Peter Anvin
  0 siblings, 0 replies; 20+ messages in thread
From: H. Peter Anvin @ 2012-04-16  6:11 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Nick Bowler, Jiri Slaby, Phil Carmody, apw, linux-kernel

On 03/26/2012 03:03 AM, Pedro Alves wrote:
> On 03/22/2012 05:17 PM, Nick Bowler wrote:
> 
>> GCC will not normally warn about the above (unless you pass
>> -Wold-style-definition) which warns for all function definitions that
>> lack a prototype.
> 
> 
> /me asks the stupid question:
> 
> Why not make the compiler catch this instead then, with -Werror=old-style-definition?
> 

Sounds like a good thing to do.

	-hpa


^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2012-04-16  6:13 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-03-22 15:27 [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations Phil Carmody
2012-03-22 15:49 ` richard -rw- weinberger
2012-03-22 16:33   ` Joe Perches
2012-03-22 16:22 ` Jiri Slaby
2012-03-22 16:49   ` Valdis.Kletnieks
2012-03-22 16:55     ` Jiri Slaby
2012-03-22 17:00       ` Jiri Slaby
2012-03-22 17:17       ` Valdis.Kletnieks
2012-03-22 19:00         ` Joe Perches
2012-03-22 16:53   ` H. Peter Anvin
2012-03-22 16:56     ` Jiri Slaby
2012-03-22 17:48     ` Phil Carmody
2012-03-22 19:10       ` Peter Seebach
2012-03-22 20:01         ` Phil Carmody
2012-03-22 17:17   ` Nick Bowler
2012-03-22 17:19     ` Nick Bowler
2012-03-26 10:03     ` Pedro Alves
2012-04-16  6:11       ` H. Peter Anvin
2012-03-22 17:32   ` Phil Carmody
2012-04-15 18:18   ` Phil Carmody

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.