[PATCH] pedantic code cleanup - am I wasting my time with this?

[PATCH] pedantic code cleanup - am I wasting my time with this?

[Jesper Juhl]

Hi people,

I'm reading through various pieces of source code to try and get an 
understanding of how the kernel works (with the hope that I'll 
eventually be able to contribute something really usefull, but you've 
got to start somewhere ;)

While reading through the source I've stumbled across various bits and 
pieces that are not exactely wrong, but not strictly correct either. I 
was wondering if I would be wasting my time by cleaning this up or if it 
would actually be appreciated. One example of these things is the patch 
below:

--- linux-2.4.3-vanilla/include/linux/rtnetlink.h       Sun Apr 22 
02:29:20 2001
+++ linux-2.4.3/include/linux/rtnetlink.h       Mon Apr 23 17:09:02 2001
@@ -112,7 +112,7 @@
         RTN_PROHIBIT,           /* Administratively prohibited  */
         RTN_THROW,              /* Not in this table            */
         RTN_NAT,                /* Translate this address       */
-       RTN_XRESOLVE,           /* Use external resolver        */
+       RTN_XRESOLVE            /* Use external resolver        */
  };

  #define RTN_MAX RTN_XRESOLVE
@@ -278,7 +278,7 @@
  #define RTAX_CWND RTAX_CWND
         RTAX_ADVMSS,
  #define RTAX_ADVMSS RTAX_ADVMSS
-       RTAX_REORDERING,
+       RTAX_REORDERING
  #define RTAX_REORDERING RTAX_REORDERING
  };

@@ -501,7 +501,7 @@
         TCA_OPTIONS,
         TCA_STATS,
         TCA_XSTATS,
-       TCA_RATE,
+       TCA_RATE
  };

  #define TCA_MAX TCA_RATE


All the above does is to remove the last comma from 3 enumeration lists. 
I know that gcc has no problem with that, but to be strictly correct the 
last entry should not have a trailing comma.

Another example is the following line (1266) from linux/include/net/sock.h

         return (waitall ? len : min(sk->rcvlowat, len)) ? : 1;

To be strictly correct the second expression (between '?' and ':' ) 
should not be omitted (all you guys already know that ofcourse).

Would patches that clean up stuff like that be appreciated or am I just 
wasting my time?
Should I just adopt an 'if gcc -Wall does not complain then it's ok' 
attitude and leave this stuff alone?


Best regards,
Jesper Juhl - juhl@eisenstein.dk

Re: [PATCH] pedantic code cleanup - am I wasting my time with this?

[Jeff Garzik]

Jesper Juhl wrote:
> 
> Hi people,
> 
> I'm reading through various pieces of source code to try and get an
> understanding of how the kernel works (with the hope that I'll
> eventually be able to contribute something really usefull, but you've
> got to start somewhere ;)
> 
> While reading through the source I've stumbled across various bits and
> pieces that are not exactely wrong, but not strictly correct either. I
> was wondering if I would be wasting my time by cleaning this up or if it
> would actually be appreciated. One example of these things is the patch
> below:
> 
> --- linux-2.4.3-vanilla/include/linux/rtnetlink.h       Sun Apr 22
> 02:29:20 2001
> +++ linux-2.4.3/include/linux/rtnetlink.h       Mon Apr 23 17:09:02 2001
> @@ -112,7 +112,7 @@
>          RTN_PROHIBIT,           /* Administratively prohibited  */
>          RTN_THROW,              /* Not in this table            */
>          RTN_NAT,                /* Translate this address       */
> -       RTN_XRESOLVE,           /* Use external resolver        */
> +       RTN_XRESOLVE            /* Use external resolver        */
>   };
> 
>   #define RTN_MAX RTN_XRESOLVE
> @@ -278,7 +278,7 @@
>   #define RTAX_CWND RTAX_CWND
>          RTAX_ADVMSS,
>   #define RTAX_ADVMSS RTAX_ADVMSS
> -       RTAX_REORDERING,
> +       RTAX_REORDERING
>   #define RTAX_REORDERING RTAX_REORDERING
>   };
> 
> @@ -501,7 +501,7 @@
>          TCA_OPTIONS,
>          TCA_STATS,
>          TCA_XSTATS,
> -       TCA_RATE,
> +       TCA_RATE

These patches increase the possibility of errors when adding future
items to these lists.

-- 
Jeff Garzik      | The difference between America and England is that
Building 1024    | the English think 100 miles is a long distance and
MandrakeSoft     | the Americans think 100 years is a long time.
                 |      (random fortune)

Re: [PATCH] pedantic code cleanup - am I wasting my time with this?

[Richard Gooch]

Jesper Juhl writes:
> Hi people,
> 
> I'm reading through various pieces of source code to try and get an 
> understanding of how the kernel works (with the hope that I'll 
> eventually be able to contribute something really usefull, but you've 
> got to start somewhere ;)
> 
> While reading through the source I've stumbled across various bits and 
> pieces that are not exactely wrong, but not strictly correct either. I 
> was wondering if I would be wasting my time by cleaning this up or if it 
> would actually be appreciated. One example of these things is the patch 
> below:
[...]
> All the above does is to remove the last comma from 3 enumeration
> lists.  I know that gcc has no problem with that, but to be strictly
> correct the last entry should not have a trailing comma.

But it's more people-friendly to have that trailing comma. It makes
adding new enumerations just slightly easier, and also makes it easier
to manually apply failed patches. I'd rather see those trailing commas
left in.

> Another example is the following line (1266) from linux/include/net/sock.h
> 
>          return (waitall ? len : min(sk->rcvlowat, len)) ? : 1;
> 
> To be strictly correct the second expression (between '?' and ':' ) 
> should not be omitted (all you guys already know that ofcourse).

Yeah, that one's pretty ugly and unreadable.

> Would patches that clean up stuff like that be appreciated or am I just 
> wasting my time?

Go ahead and make suggestions. I expect some things will be accepted,
some rejected (just like I did). Steer clear of any brace or tabbing
style changes, though.

> Should I just adopt an 'if gcc -Wall does not complain then it's ok' 
> attitude and leave this stuff alone?

Gcc is often too picky. It tries to be clever, but in many cases I've
found it's not clever enough. It's useful as a mechanism to identify
potential problems, but just because gcc whinges, doesn't mean there
is a problem. You'll need to read and analyse the code to be sure gcc
is correct in throwing a warning.

The goal should *not* be to shut up gcc. The goal should be to produce
more readable code and to fix bugs. Gcc is merely a tool. And a flawed
one, at that.

				Regards,

					Richard....
Permanent: rgooch@atnf.csiro.au
Current:   rgooch@ras.ucalgary.ca

Re: [PATCH] pedantic code cleanup - am I wasting my time with this?

[Jesper Juhl]

Richard Gooch wrote:

> Jesper Juhl writes:
>
>> All the above does is to remove the last comma from 3 enumeration
>> lists.  I know that gcc has no problem with that, but to be strictly
>> correct the last entry should not have a trailing comma.
> 
> 
> But it's more people-friendly to have that trailing comma. It makes
> adding new enumerations just slightly easier, and also makes it easier
> to manually apply failed patches. I'd rather see those trailing commas
> left in.

> 

You are right. As several people have pointed out to me it is in fact 
legal to have the trailing comma. And it _does_ make it easier to add 
new lines.
At least I have learned a lesson here; be 100% sure of your facts before 
posting to linux-kernel ;)

> 
>> Another example is the following line (1266) from linux/include/net/sock.h
>> 
>>          return (waitall ? len : min(sk->rcvlowat, len)) ? : 1;
>> 
>> To be strictly correct the second expression (between '?' and ':' ) 
>> should not be omitted (all you guys already know that ofcourse).
> 
> 
> Yeah, that one's pretty ugly and unreadable.
> 

That function (sock_rcvlowat()) only gets called a few places, so I'll 
see if I can figure out exactely what's going on and come up with a 
better construct (it might take me ages, but I'm determined to learn to 
find my way around this code)...

> 
> Go ahead and make suggestions. I expect some things will be accepted,
> some rejected (just like I did). Steer clear of any brace or tabbing
> style changes, though.
> 

Ok, I'll continue reading code and keep my eyes open for these things.

[...]

> The goal should *not* be to shut up gcc. The goal should be to produce
> more readable code and to fix bugs. Gcc is merely a tool. And a flawed
> one, at that.
> 

I'll remember that. Thank you to everyone who have taken their time to 
answer my post, you have all been very helpfull!

- Jesper Juhl - juhl@eisenstein.dk

Re: [PATCH] pedantic code cleanup - am I wasting my time with this?

[Sean Hunter]

On Mon, Apr 23, 2001 at 05:26:27PM +0200, Jesper Juhl wrote:
> All the above does is to remove the last comma from 3 enumeration lists. 
> I know that gcc has no problem with that, but to be strictly correct the 
> last entry should not have a trailing comma.
> 

Sadly not.  This isn't a gcc thing: ANSI says that trailing comma is ok (K&R
Second edition, A8.7 - pg 218 &219 in my copy)

Sean

Re: [PATCH] pedantic code cleanup - am I wasting my time with this?

[Jesper Juhl]

Sean Hunter wrote:

> On Mon, Apr 23, 2001 at 05:26:27PM +0200, Jesper Juhl wrote:
>> last entry should not have a trailing comma.
>> 
> 
> Sadly not.  This isn't a gcc thing: ANSI says that trailing comma is ok (K&R
> Second edition, A8.7 - pg 218 &219 in my copy)
> 

You are right, I just consulted my own copy, and nothing strictly 
forbids the comma... Sorry about that, I should have been more thorough 
before reporting that one...

- Jesper Juhl

Re: [PATCH] pedantic code cleanup - am I wasting my time with this?

[Rusty Russell]

In message <3AE449A3.3050601@eisenstein.dk> you write:
>          return (waitall ? len : min(sk->rcvlowat, len)) ? : 1;
> 
> To be strictly correct the second expression (between '?' and ':' ) 
> should not be omitted (all you guys already know that ofcourse).

It's a GCC extension.  From Documentation/DocBook/kernel-hacking.tmpl:

    GNU Extensions are explicitly allowed in the Linux kernel.

Rusty.
--
Premature optmztion is rt of all evl. --DK

Re: [PATCH] pedantic code cleanup - am I wasting my time with this?

[Stephen Satchell]

At 05:58 PM 4/23/01 +0200, you wrote:
>>On Mon, Apr 23, 2001 at 05:26:27PM +0200, Jesper Juhl wrote:
>>>last entry should not have a trailing comma.
>>Sadly not.  This isn't a gcc thing: ANSI says that trailing comma is ok (K&R
>>Second edition, A8.7 - pg 218 &219 in my copy)
>
>You are right, I just consulted my own copy, and nothing strictly forbids 
>the comma... Sorry about that, I should have been more thorough before 
>reporting that one...

 From the X3J11 Rationale document, paraphrase:  The inclusion of optional 
trailing commas is to ease the task of generating code by automatic 
programs such as LEX and YACC.

Satch