pedantic code cleanup - am I wasting my time with this?
PATCH 1/1 pedantic code cleanup - am I wasting my time with this?
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:
include/linux/rtnetlink.h Mon Apr 23 17:09:02 2001 renamed from include/linux/rtnetlink.h Sun Apr 22 02:29:20 2001 +3 -3
@@ -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 - [email protected]
Quoted 36 lines · 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.hSun Apr 22
02:29:20 2001
+++linux-2.4.3/include/linux/rtnetlink.hMon Apr 23 17:09:02 2001include/linux/rtnetlink.h Mon Apr 23 17:09:02 2001 renamed from include/linux/rtnetlink.h Sun Apr 22 02:29:20 2001 +2
-2@@ -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 };
include/linux/rtnetlink.h Mon Apr 23 17:09:02 2001 line 504
Quoted 5 lines
include/linux/rtnetlink.h Mon Apr 23 17:09:02 2001 renamed from include/linux/rtnetlink.h Sun Apr 22 02:29:20 2001 +1
-1@@ -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.
Signature · 4 lines
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)Quoted 12 lines · 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:
[...]
Quoted 3 lines
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.
Quoted 6 lines
Another example is the following line (1266) from
linux/include/net/sock.hreturn (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.
Quoted 2 lines
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.
Quoted 2 lines
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: [email protected] Current: [email protected]
Quoted 4 lines · Jesper Juhl
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
Quoted 7 lines · Sean Hunter
Quoted 2 lines · Jesper Juhl
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
Quoted 7 lines · Richard Gooch
Quoted 3 lines · Jesper Juhl
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.
Quoted 6 lines
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 ;)
Quoted 10 lines
Quoted 6 lines
Another example is the following line (1266) from
linux/include/net/sock.hreturn (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)...
Quoted 5 lines
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.
[...]
Quoted 4 lines
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 - [email protected]
In message <[email protected]> you write:
Quoted 4 lines
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