--- ./lib/kobject_uevent.c.orig 2005-04-05 16:39:09.000000000 +0100
+++ ./lib/kobject_uevent.c 2005-04-05 17:01:26.000000000 +0100
@@ -234,10 +234,9 @@ void kobject_hotplug(struct kobject *kob
if (!action_string)
return;
- envp = kmalloc(NUM_ENVP * sizeof (char *), GFP_KERNEL);
+ envp = kmalloc_zero(NUM_ENVP * sizeof (char *), GFP_KERNEL);
if (!envp)
return;
- memset (envp, 0x00, NUM_ENVP * sizeof (char *));
buffer = kmalloc(BUFFER_SIZE, GFP_KERNEL);
if (!buffer)
On Tue, 5 April 2005 17:26:31 +0100, Paulo Marques wrote:
>
> Would this be a good thing to clean up, or isn't it worth the effort at all?
I would welcome such a stream of patches. But in spite of the calloc
interface being rather stupid, I'd prefer to see patches with kcalloc
instead of kmalloc_zero.
> --- ./lib/kobject_uevent.c.orig 2005-04-05 16:39:09.000000000 +0100
> +++ ./lib/kobject_uevent.c 2005-04-05 17:01:26.000000000 +0100
> @@ -234,10 +234,9 @@ void kobject_hotplug(struct kobject *kob
> if (!action_string)
> return;
>
> - envp = kmalloc(NUM_ENVP * sizeof (char *), GFP_KERNEL);
> + envp = kmalloc_zero(NUM_ENVP * sizeof (char *), GFP_KERNEL);
> if (!envp)
> return;
> - memset (envp, 0x00, NUM_ENVP * sizeof (char *));
>
> buffer = kmalloc(BUFFER_SIZE, GFP_KERNEL);
> if (!buffer)
J?rn
--
There's nothing better for promoting creativity in a medium than
making an audience feel "Hmm ? I could do better than that!"
-- Douglas Adams in a slashdot interview
On Tue, 5 Apr 2005, Paulo Marques wrote:
> Hi,
> I noticed there are a number of places in the kernel that do:
> ptr = kmalloc(n * size, ...)
> if (!ptr)
> goto out;
> memset(ptr, 0, n * size);
> It seems that these could be replaced by:
> ptr = kcalloc(n, size, ...)
> if (!ptr)
> goto out;
or simply
if (!(ptr = kcalloc(n, size, ...)))
goto out;
and save an additional line of screen realestate while you are at it...
--
Jesper Juhl
> or simply
> if (!(ptr = kcalloc(n, size, ...)))
> goto out;
> and save an additional line of screen realestate while you are at it...
No, please don't do that. The general kernel style is to avoid
assignments within conditionals.
- R.
On Tue, 5 Apr 2005, Roland Dreier wrote:
> > or simply
> > if (!(ptr = kcalloc(n, size, ...)))
> > goto out;
> > and save an additional line of screen realestate while you are at it...
>
> No, please don't do that. The general kernel style is to avoid
> assignments within conditionals.
>
It may be the prefered style to avoid assignments in conditionals, but in
that case we have a lot of cleanup to do. What I wrote above is quite
common in the current tree - a simple egrep -r "if\ *\(\!\(.+=" * in
2.6.12-rc2-mm1 will find you somewhere between 1000 and 2000 cases
scattered all over the tree.
Personally I don't see why thy should not be used. They are short, not any
harder to read (IMHO), save screen space & are quite common in userspace
code as well (so people should be used to seeing them).
If such statements are generally frawned upon then I'd suggest an addition
be made to Documentation/CodingStyle mentioning that fact, and I wonder if
patches to clean up current users would be welcome?
--
Jesper Juhl
On Tue, 5 April 2005 22:01:49 +0200, Jesper Juhl wrote:
> On Tue, 5 Apr 2005, Roland Dreier wrote:
>
> > > or simply
> > > if (!(ptr = kcalloc(n, size, ...)))
> > > goto out;
> > > and save an additional line of screen realestate while you are at it...
> >
> > No, please don't do that. The general kernel style is to avoid
> > assignments within conditionals.
> >
> It may be the prefered style to avoid assignments in conditionals, but in
> that case we have a lot of cleanup to do. What I wrote above is quite
> common in the current tree - a simple egrep -r "if\ *\(\!\(.+=" * in
> 2.6.12-rc2-mm1 will find you somewhere between 1000 and 2000 cases
> scattered all over the tree.
>
> Personally I don't see why thy should not be used. They are short, not any
> harder to read (IMHO), save screen space & are quite common in userspace
> code as well (so people should be used to seeing them).
>
> If such statements are generally frawned upon then I'd suggest an addition
> be made to Documentation/CodingStyle mentioning that fact, and I wonder if
> patches to clean up current users would be welcome?
I _do_ change them whenever they occur in code I maintain. And each
time, it is an improvement.
o Functional code always has the same indentation. I can mentally
ignore the error path by ignoring all indented code. Getting a
quick overview is quite nice.
o Rather often, your preferred variant violates the 80 columns rule.
If I need the line break anyway,...
o Keeping condition and functional code seperate avoids the Lisp-style
bracket maze. Some editors can help you here, but not needing any
help would be even better, no?
J?rn
--
Mundie uses a textbook tactic of manipulation: start with some
reasonable talk, and lead the audience to an unreasonable conclusion.
-- Bruce Perens
On Tuesday 05 April 2005 21:00, J?rn Engel wrote:
> On Tue, 5 April 2005 17:26:31 +0100, Paulo Marques wrote:
> >
> > Would this be a good thing to clean up, or isn't it worth the effort at all?
>
> I would welcome such a stream of patches. But in spite of the calloc
> interface being rather stupid, I'd prefer to see patches with kcalloc
> instead of kmalloc_zero.
kcalloc call will have three params pushed on stack. in 99% of cases
it could be done with two. If anyone's going to do it, please create
and use
void *kzalloc(size, gfp_mask)
or zalloc, kmalloc_zero... whatever.
--
vda
J?rn Engel wrote:
> On Tue, 5 April 2005 22:01:49 +0200, Jesper Juhl wrote:
>
>>On Tue, 5 Apr 2005, Roland Dreier wrote:
>>
>>
>>> > or simply
>>> > if (!(ptr = kcalloc(n, size, ...)))
>>> > goto out;
>>> > and save an additional line of screen realestate while you are at it...
>>>
>>>No, please don't do that. The general kernel style is to avoid
>>>assignments within conditionals.
FWIW, I also agree that assignments within conditionals are evil and
hurt readability a lot, for no actual benefit.
Since one of the advantages of this cleanup is improve readability, it
would be counterproductive to do this.
To explain why this cleanup improves readability take the following
sample from sound/usb/usx2y/usbusx2yaudio.c (a random sample):
> us = kmalloc(sizeof(*us) + sizeof(struct urb*) * NOOF_SETRATE_URBS, GFP_KERNEL);
> if (NULL == us) {
> err = -ENOMEM;
> goto cleanup;
> }
> memset(us, 0, sizeof(*us) + sizeof(struct urb*) * NOOF_SETRATE_URBS);
In this case you have to read the size portion of the kmalloc and the
memset carefully to make sure that they are exactly the same, whereas in
code like this:
> us = kcalloc(1, sizeof(*us) + sizeof(struct urb*) * NOOF_SETRATE_URBS, GFP_KERNEL);
> if (NULL == us) {
> err = -ENOMEM;
> goto cleanup;
> }
it is self-evident that the whole area is being cleared. It also leaves
a smaller room for mistakes.
I still don't like the kcalloc API with a "1" argument. Not only most of
these allocations fall into that case, but also kmalloc_zero (or
kmalloc_zeroed) is much more clear than kcalloc that changes just one
letter from kmalloc. Someone reading fast through the code may kcalloc
as if it were kmalloc.
More over, passing an extra parameter waste a few more bytes of code. I
know is not much, but if the cleanup will address hundreds of these then
it starts to be something to consider.
However "calloc" is the standard C interface for doing this, so it makes
some sense to use it here as well... :(
--
Paulo Marques - http://www.grupopie.com
All that is necessary for the triumph of evil is that good men do nothing.
Edmund Burke (1729 - 1797)
Hi,
On Apr 6, 2005 3:15 PM, Paulo Marques <[email protected]> wrote:
> However "calloc" is the standard C interface for doing this, so it makes
> some sense to use it here as well... :(
I initally submitted kcalloc() with just one parameter but Arjan
wanted it to be similar to standard calloc() so we could check for
overflows. I don't see any reason not to introduce kzalloc() for the
common case you mentioned (as suggested by Denis).
Pekka
Pekka Enberg wrote:
> Hi,
>
> On Apr 6, 2005 3:15 PM, Paulo Marques <[email protected]> wrote:
>
>>However "calloc" is the standard C interface for doing this, so it makes
>>some sense to use it here as well... :(
>
>
> I initally submitted kcalloc() with just one parameter but Arjan
> wanted it to be similar to standard calloc() so we could check for
> overflows. I don't see any reason not to introduce kzalloc() for the
> common case you mentioned (as suggested by Denis).
kzalloc it is, then.
By the way I did a quick measurement to see how much we could gain in
kernel size by doing this. This is with a 2.6.11-rc2, defconfig kernel:
with kmalloc+memset:
vmlinuz: 5521614
bzImage: 2005274
with kzalloc:
vmlinuz: 5513422
bzImage: 2003927
So we gain 8kB on the uncompressed image and 1347 bytes on the
compressed one. This was just a dumb test and actual results might be
better due to smarter human cleanups.
Not a spectacular gain per se, but the increase in code readability is
still worth it, IMHO.
--
Paulo Marques - http://www.grupopie.com
All that is necessary for the triumph of evil is that good men do nothing.
Edmund Burke (1729 - 1797)
On Tue, Apr 05, 2005 at 05:26:31PM +0100, Paulo Marques wrote:
>
> Hi,
Hi Paulo,
> I noticed there are a number of places in the kernel that do:
>
> ptr = kmalloc(n * size, ...)
> if (!ptr)
> goto out;
> memset(ptr, 0, n * size);
>
> It seems that these could be replaced by:
>
> ptr = kcalloc(n, size, ...)
> if (!ptr)
> goto out;
>
> saving a few bytes.
>...
> A quick (and lame) grep through the tree shows about 1200 of these
>cases. This means that about one quarter of all the kmallocs in the
>kernel are actually zeroed right after allocation.
>...
> pros:
> - smaller kernel image size
> - smaller (and more readable) source code
>...
Which is better readable depends on what you are used to.
> cons:
> - the NULL test is done twice
> - memset will not be optimized for constant sizes
>...
> Would this be a good thing to clean up, or isn't it worth the effort at all?
>...
You do plan to patch 1200 places in the kernel for this
micro-optimization?
This sounds like a really big overhead for a pretty small gain.
There are tasks of higher value that can be done.
E.g. read my "Stack usage tasks" email. The benefits would only be
present for people using GNU gcc 3.4 or SuSE gcc 3.3 on i386, but this
is a reasonable subset of the kernel users - and it brings them a
2% kernel size improvement.
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
On Apr 06, 2005, at 11:50, Paulo Marques wrote:
> kzalloc it is, then.
>
> [...]
>
> So we gain 8kB on the uncompressed image and 1347 bytes on the
> compressed one. This was just a dumb test and actual results might be
> better due to smarter human cleanups.
>
> Not a spectacular gain per se, but the increase in code readability is
> still worth it, IMHO.
Perhaps this could eventually be modified to draw from a prezeroed
block of
memory, similar to the current code for doing the same thing for
userspace.
It probably wouldn't give much performance gain, especially since it's
not
used for large blocks or large numbers of small objects (As you would
use a
slabcache for those), but it might help a bit. Of course, the code
would
need to fall back quickly if such an allocation would be messy or
expensive
for any reason.
Cheers,
Kyle Moffett
-----BEGIN GEEK CODE BLOCK-----
Version: 3.12
GCM/CS/IT/U d- s++: a18 C++++>$ UB/L/X/*++++(+)>$ P+++(++++)>$
L++++(+++) E W++(+) N+++(++) o? K? w--- O? M++ V? PS+() PE+(-) Y+
PGP+++ t+(+++) 5 X R? tv-(--) b++++(++) DI+ D+ G e->++++$ h!*()>++$ r
!y?(-)
------END GEEK CODE BLOCK------
Adrian Bunk wrote:
> On Tue, Apr 05, 2005 at 05:26:31PM +0100, Paulo Marques wrote:
>>[...]
>
> Hi Paulo,
Hi Adrian,
>>[...]
>>pros:
>> - smaller kernel image size
>> - smaller (and more readable) source code
>
> Which is better readable depends on what you are used to.
That's true to some degree, but look at code like this (in
drivers/usb/input/hid-core.c):
> if (!(field = kmalloc(sizeof(struct hid_field) + usages * sizeof(struct hid_usage)
> + values * sizeof(unsigned), GFP_KERNEL))) return NULL;
>
> memset(field, 0, sizeof(struct hid_field) + usages * sizeof(struct hid_usage)
> + values * sizeof(unsigned));
and compare to this:
> field = kzalloc(sizeof(struct hid_field) + usages * sizeof(struct hid_usage)
> + values * sizeof(unsigned), GFP_KERNEL);
> if (!field)
> return NULL;
In the first case you have to read carefully to make sure that the size
argument in both the kmalloc and the memset are the same. Even more, if
the size needs to be updated to include something more, a mistake can be
made by inserting the extra size just in the kmalloc call. Also, you are
assuming that the compiler is smart enough to notice that the two
expressions are the same and cache the result, but this is probably true
for gcc, anyway.
I think most will agree that the second piece of code is more "readable".
>>cons:
>> - the NULL test is done twice
>> - memset will not be optimized for constant sizes
>>...
>>Would this be a good thing to clean up, or isn't it worth the effort at all?
>
> You do plan to patch 1200 places in the kernel for this
> micro-optimization?
I was actually planning on sending a patch at a time for a reasonably
sized subsection. Like "use kzalloc in usb/serial drivers", or "use
kzalloc in sound/core", etc.
This way, it shouldn't be more than 1150 patches ;)
> This sounds like a really big overhead for a pretty small gain.
Yes it is. But at least it will remove 1000+ lines of redundant kernel code.
I really don't see this as a priority at all. I'll probably submit a
patch shortly to create the kzalloc function. This way developers become
aware that it exists and that it should be used, and we don't get a lot
of new code with the same constructs.
The cleanup can then progress at a slowly pace, without breaking things
and without producing too much merging problems.
> There are tasks of higher value that can be done.
I couldn't agree more :)
> E.g. read my "Stack usage tasks" email. The benefits would only be
> present for people using GNU gcc 3.4 or SuSE gcc 3.3 on i386, but this
> is a reasonable subset of the kernel users - and it brings them a
> 2% kernel size improvement.
I've read that thread, but it seems that it is at a dead end right now,
since we don't have a good tool to find out which functions are abusing
the stack with unit-at-a-time.
Is there some way to even limit the search, like using a stack usage log
from a compilation without unit-at-a-time, and going over the hotspots
to check for problems?
If we can get a list, even if it contains a lot of false positives, I
would more than happy to help out...
--
Paulo Marques - http://www.grupopie.com
All that is necessary for the triumph of evil is that good men do nothing.
Edmund Burke (1729 - 1797)
On Fri, 8 April 2005 13:38:22 +0100, Paulo Marques wrote:
> Adrian Bunk wrote:
>
> >E.g. read my "Stack usage tasks" email. The benefits would only be
> >present for people using GNU gcc 3.4 or SuSE gcc 3.3 on i386, but this
> >is a reasonable subset of the kernel users - and it brings them a
> >2% kernel size improvement.
>
> I've read that thread, but it seems that it is at a dead end right now,
> since we don't have a good tool to find out which functions are abusing
> the stack with unit-at-a-time.
>
> Is there some way to even limit the search, like using a stack usage log
> from a compilation without unit-at-a-time, and going over the hotspots
> to check for problems?
>
> If we can get a list, even if it contains a lot of false positives, I
> would more than happy to help out...
The situation is bad, but not that bad.
As a starter, you can compile the kernel with gcc 3.4 and run "make
checkstack" on it. No call graph information in there, but getting
all functions on the list below 2k would help.
Next step would be a small hack to my private tool to read stack
consumption from gcc 3.4 and call graph from gcc 3.1. Obviously you
get tons of "these five functions in the call graph are actually a
single one in the real binary". Quick and dirty.
Then someone (doesn't have to be me) should spend some time to port
the callgraph extraction code from 3.1 to 3.4 or 4.0. Before the
unit-at-a-time thing came up, I wanted to port things to sparse. But
sparse would suffer from the same problem of not inlining functions
identically to the real compiler, so current gcc appears to be a
better target.
Step 1 is possible right now, step 2 might take a while (i'm on
vacation) and step 3 may not happen this year anymore, unless someone
else wants to start hacking on it.
J?rn
--
Courage is not the absence of fear, but rather the judgement that
something else is more important than fear.
-- Ambrose Redmoon
On Fri, Apr 08, 2005 at 01:38:22PM +0100, Paulo Marques wrote:
> Adrian Bunk wrote:
> >On Tue, Apr 05, 2005 at 05:26:31PM +0100, Paulo Marques wrote:
>
> Hi Adrian,
Hi Paolo,
> >>[...]
> >>pros:
> >> - smaller kernel image size
> >> - smaller (and more readable) source code
> >
> >Which is better readable depends on what you are used to.
>
> That's true to some degree, but look at code like this (in
> drivers/usb/input/hid-core.c):
>
> > if (!(field = kmalloc(sizeof(struct hid_field) + usages *
> > sizeof(struct hid_usage)
> > + values * sizeof(unsigned), GFP_KERNEL))) return NULL;
> >
> > memset(field, 0, sizeof(struct hid_field) + usages * sizeof(struct
> > hid_usage)
> > + values * sizeof(unsigned));
>
> and compare to this:
>
> > field = kzalloc(sizeof(struct hid_field) + usages * sizeof(struct
> > hid_usage)
> > + values * sizeof(unsigned), GFP_KERNEL);
> > if (!field)
> > return NULL;
>
> In the first case you have to read carefully to make sure that the size
> argument in both the kmalloc and the memset are the same. Even more, if
> the size needs to be updated to include something more, a mistake can be
> made by inserting the extra size just in the kmalloc call. Also, you are
> assuming that the compiler is smart enough to notice that the two
> expressions are the same and cache the result, but this is probably true
> for gcc, anyway.
>
> I think most will agree that the second piece of code is more "readable".
In this case yes (but it could still use the normal kcalloc).
>...
> >There are tasks of higher value that can be done.
>
> I couldn't agree more :)
>
> >E.g. read my "Stack usage tasks" email. The benefits would only be
> >present for people using GNU gcc 3.4 or SuSE gcc 3.3 on i386, but this
> >is a reasonable subset of the kernel users - and it brings them a
> >2% kernel size improvement.
>
> I've read that thread, but it seems that it is at a dead end right now,
> since we don't have a good tool to find out which functions are abusing
> the stack with unit-at-a-time.
>
> Is there some way to even limit the search, like using a stack usage log
> from a compilation without unit-at-a-time, and going over the hotspots
> to check for problems?
>
> If we can get a list, even if it contains a lot of false positives, I
> would more than happy to help out...
Joerg's list of recursions should be valid independent of the kernel
version. Fixing any real stack problems [1] that might be in this list
is a valuable task.
And "make checkstack" in a kernel compiled with unit-at-a-time lists
several possible problems at the top.
> Paulo Marques
cu
Adrian
[1] there are cases in this list that aren't a problem e.g. because of
an obviously limited recursion
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
On Fri, 8 April 2005 15:00:08 +0200, Adrian Bunk wrote:
>
> Joerg's
Please! ;)
J?rn
--
It's just what we asked for, but not what we want!
-- anonymous
On Fri, Apr 08, 2005 at 03:20:52PM +0200, J?rn Engel wrote:
> On Fri, 8 April 2005 15:00:08 +0200, Adrian Bunk wrote:
> >
> > Joerg's
>
> Please! ;)
Ups, sorry...
> J?rn
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
Adrian Bunk wrote:
>[...]
>>>On Tue, Apr 05, 2005 at 05:26:31PM +0100, Paulo Marques wrote:
>>
>>Hi Adrian,
>
> Hi Paolo,
Paulo, please :)
Paolo is Spanish (or Italian), whereas Paulo is a Portuguese name.
>>[...]
>>I think most will agree that the second piece of code is more "readable".
>
> In this case yes (but it could still use the normal kcalloc).
Yes, but the cleanup is already just barely justifiable. 75% (or more)
of the calls will use just one parameter. Adding a stupid "1" to all of
those will make the cleanup even less worth while.
>[...]
> Joerg's list of recursions should be valid independent of the kernel
> version. Fixing any real stack problems [1] that might be in this list
> is a valuable task.
>
> And "make checkstack" in a kernel compiled with unit-at-a-time lists
> several possible problems at the top.
Ok, I've read J?rn's mail also and I think I can help out. It seems
however that there are more people working on this. Will it be better to
coordinate so we don't duplicate efforts or is the "everyone looks at
everything" approach better, so that its harder to miss something?
--
Paulo Marques - http://www.grupopie.com
All that is necessary for the triumph of evil is that good men do nothing.
Edmund Burke (1729 - 1797)
On Fri, Apr 08, 2005 at 05:24:42PM +0100, Paulo Marques wrote:
> Adrian Bunk wrote:
> >[...]
> >>>On Tue, Apr 05, 2005 at 05:26:31PM +0100, Paulo Marques wrote:
> >>
> >>Hi Adrian,
> >
> >Hi Paolo,
>
> Paulo, please :)
>...
The second name I got wrong today...
Sorry.
>...
> >Joerg's list of recursions should be valid independent of the kernel
> >version. Fixing any real stack problems [1] that might be in this list
> >is a valuable task.
> >
> >And "make checkstack" in a kernel compiled with unit-at-a-time lists
> >several possible problems at the top.
>
> Ok, I've read J?rn's mail also and I think I can help out. It seems
> however that there are more people working on this. Will it be better to
> coordinate so we don't duplicate efforts or is the "everyone looks at
> everything" approach better, so that its harder to miss something?
The only other person that seemed very interested n stack issues was
Yum Rayan <[email protected]>.
You could coordinate with him, but in the end it should be possible to
have a first set of patches ready a few hours or even minutes after you
started, so duplicate efforts would require a very unlucky timing.
> Paulo Marques
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
On Fri, 8 Apr 2005 21:43:55 +0200 Adrian Bunk wrote:
| On Fri, Apr 08, 2005 at 05:24:42PM +0100, Paulo Marques wrote:
| > Adrian Bunk wrote:
| > >[...]
| > >>>On Tue, Apr 05, 2005 at 05:26:31PM +0100, Paulo Marques wrote:
| > >>
| > >>Hi Adrian,
| > >
| >...
| > >Joerg's list of recursions should be valid independent of the kernel
| > >version. Fixing any real stack problems [1] that might be in this list
| > >is a valuable task.
| > >
| > >And "make checkstack" in a kernel compiled with unit-at-a-time lists
| > >several possible problems at the top.
| >
| > Ok, I've read J?rn's mail also and I think I can help out. It seems
| > however that there are more people working on this. Will it be better to
| > coordinate so we don't duplicate efforts or is the "everyone looks at
| > everything" approach better, so that its harder to miss something?
|
| The only other person that seemed very interested n stack issues was
| Yum Rayan <[email protected]>.
Well, I am, but they are not high on my list right now,
so no coordination is needed with me currently.
| You could coordinate with him, but in the end it should be possible to
| have a first set of patches ready a few hours or even minutes after you
| started, so duplicate efforts would require a very unlucky timing.
|
| > Paulo Marques
|
| cu
| Adrian
---
~Randy
On Wed, 6 Apr 2005, J?rn Engel wrote:
> On Tue, 5 April 2005 22:01:49 +0200, Jesper Juhl wrote:
> > On Tue, 5 Apr 2005, Roland Dreier wrote:
> >
> > > > or simply
> > > > if (!(ptr = kcalloc(n, size, ...)))
> > > > goto out;
> > > > and save an additional line of screen realestate while you are at it...
> > >
> > > No, please don't do that. The general kernel style is to avoid
> > > assignments within conditionals.
> > >
> > It may be the prefered style to avoid assignments in conditionals, but in
> > that case we have a lot of cleanup to do. What I wrote above is quite
> > common in the current tree - a simple egrep -r "if\ *\(\!\(.+=" * in
> > 2.6.12-rc2-mm1 will find you somewhere between 1000 and 2000 cases
> > scattered all over the tree.
> >
> > Personally I don't see why thy should not be used. They are short, not any
> > harder to read (IMHO), save screen space & are quite common in userspace
> > code as well (so people should be used to seeing them).
> >
> > If such statements are generally frawned upon then I'd suggest an addition
> > be made to Documentation/CodingStyle mentioning that fact, and I wonder if
> > patches to clean up current users would be welcome?
>
> I _do_ change them whenever they occur in code I maintain. And each
> time, it is an improvement.
>
> o Functional code always has the same indentation. I can mentally
> ignore the error path by ignoring all indented code. Getting a
> quick overview is quite nice.
>
> o Rather often, your preferred variant violates the 80 columns rule.
> If I need the line break anyway,...
>
> o Keeping condition and functional code seperate avoids the Lisp-style
> bracket maze. Some editors can help you here, but not needing any
> help would be even better, no?
>
Ok, I accept those arguments, they make sense. I may still have a personal
preference that differ slightly, but I see your point(s).
--
Jesper
Paulo wrote:
> In the first case you have to read carefully to make sure that the size
> argument in both the kmalloc and the memset are the same.
If that were the only concern (which it isn't, and I don't pretend to be
addressing the other concerns on this thread) then pulling out the
common sub-expression would help readability, and reduce the chance of a
coding bug should the size change in the future.
Anytime you find yourself coding the same complex expression twice, an
alarm should go off in your mind, and you should ask yourself if there
is a reasonable way to get by with only one instance of the coding in
the source.
The following patch shows what I mean here.
It also replaces the less conventional form:
if (!(ptr = some_function(args)) do-something;
with the more conventional form:
if ((ptr = some_function(args)) == NULL)
do-something;
diff -Naurp 2.6.12-rc2.orig/drivers/usb/input/hid-core.c 2.6.12-rc2/drivers/usb/input/hid-core.c
--- 2.6.12-rc2.orig/drivers/usb/input/hid-core.c 2005-04-09 06:57:47.000000000 -0700
+++ 2.6.12-rc2/drivers/usb/input/hid-core.c 2005-04-09 07:05:14.000000000 -0700
@@ -90,17 +90,18 @@ static struct hid_report *hid_register_r
static struct hid_field *hid_register_field(struct hid_report *report, unsigned usages, unsigned values)
{
struct hid_field *field;
+ int sz;
if (report->maxfield == HID_MAX_FIELDS) {
dbg("too many fields in report");
return NULL;
}
- if (!(field = kmalloc(sizeof(struct hid_field) + usages * sizeof(struct hid_usage)
- + values * sizeof(unsigned), GFP_KERNEL))) return NULL;
-
- memset(field, 0, sizeof(struct hid_field) + usages * sizeof(struct hid_usage)
- + values * sizeof(unsigned));
+ sz = sizeof(struct hid_field) + usages * sizeof(struct hid_usage) +
+ values * sizeof(unsigned);
+ if ((field = kmalloc(sz, GFP_KERNEL)) == NULL)
+ return NULL;
+ memset(field, 0, sz);
field->index = report->maxfield++;
report->field[field->index] = field;
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.650.933.1373, 1.925.600.0401