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 - [email protected]
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)
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: [email protected]
Current: [email protected]
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
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
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 - [email protected]
In message <[email protected]> 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
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