[PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations

[PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations

[Phil Carmody]

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

Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations

[richard -rw- weinberger]

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

Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations

[Joe Perches]

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.

Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations

[Jiri Slaby]

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

Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations

[Valdis.Kletnieks]

[-- 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 --]

Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations

[Jiri Slaby]

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

Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations

[Jiri Slaby]

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

Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations

[Valdis.Kletnieks]

[-- 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 --]

Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations

[Joe Perches]

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.

Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations

[H. Peter Anvin]

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

Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations

[Jiri Slaby]

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

Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations

[Phil Carmody]

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

Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations

[Peter Seebach]

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.

Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations

[Phil Carmody]

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

Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations

[Nick Bowler]

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/)

Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations

[Nick Bowler]

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/)

Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations

[Pedro Alves]

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

Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations

[H. Peter Anvin]

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

Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations

[Phil Carmody]

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

Re: [PATCH 1/1] checkpatch.pl: thou shalt not use () or (...) in function declarations

[Phil Carmody]

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