I've written a checkpatch guide for newbies because it seems like they
make the same mistakes over and over. I intend to put it under
Documentation/. Could you look it over?
Introduction
This document is aimed at new kernel contributors using "checkpatch.pl --file".
The first thing to remember is that the aim is not to get rid of every
checkpatch.pl warning; the aim is to make the code cleaner and more readable.
The other thing to remember is that checkpatch.pl is not a very smart tool and
there are mistakes that it misses so keep your eyes open for those as well.
For example, checkpatch.pl could warn about a badly formatted warning message.
Ask yourself, is the warning message is clear? Is it needed? Could a
non-privileged user trigger the warning and flood the syslog? Are there
spelling mistakes? Since Checkpatch.pl has flagged the line as sloppy code,
there may be multiple mistakes.
In the Linux kernel, we take an enormous pride in our work and we want clean
code. But the one major drawback to cleanup patches is that they break
"git blame" so it's not a good idea for newbies to send very trivial cleanup
patches to the kernel/ directory. It's better to get a little experience in the
drivers/ directory first. The drivers/staging/ directory in particular always
needs cleanup patches.
General Hints
1) Don't put too many things in one patch because it makes it hard to review.
Break the patch up into a patch series like this made up example:
[PATCH 1/4] subsystem: driver: Use tabs to indent instead of spaces
[PATCH 2/4] subsystem: driver: Add spaces around math operations
[PATCH 3/4] subsystem: driver: Remove extra braces
[PATCH 4/4] subsystem: driver: Delete LINUX_VERSION_CODE related code
Long Lines
Historically screens were 80 characters wide and it was annoying when code went
over the edge. These days we have larger screens, but we keep the 80 character
limit because it forces us to write simpler code.
One way to remove indent levels is using functions. If you find yourself
writing a loop or a switch statement and you're already indented several tabs
then probably it should be a new function instead.
Whenever possible return immediately.
Bad:
- foo = kmalloc();
- if (foo) {
- /* code indent 2 tabs */
- }
- return foo;
Good:
+ foo = kmalloc();
+ if (!foo)
+ return NULL;
+ /* code indented 1 tab */
+ return foo;
Choose shorter names.
Bad:
- for(uiIpv6AddIndex = 0; uiIpv6AddIndex < uiIpv6AddrNoLongWords;
- uiIpv6AddIndex++) {
Good:
+ for (i = 0; i < long_words; i++) {
Use temporary variable names:
Bad:
- dev->backdev[count]->bitlistsize =
- dev->backdev[count]->devmagic->bitlistsize;
Good:
+ back = dev->backdev[count];
+ back->bitlistsize = back->devmagic->bitlistsize;
Don't do complicated things in the initializer:
Bad:
- struct binder_ref_death *tmp_death = container_of(w,
- struct binder_ref_death, work);
Good:
+ struct binder_ref_death *tmp_death;
+
+ tmp_death = container_of(w, struct binder_ref_death, work);
If you must break up a long line then align it nicely. Use spaces if needed.
Bad:
- if (adapter->flowcontrol == FLOW_RXONLY ||
- adapter->flowcontrol == FLOW_BOTH) {
Good:
+ if (adapter->flowcontrol == FLOW_RXONLY ||
+ adapter->flowcontrol == FLOW_BOTH) {
It's preferred if the operator goes at the end of the first line instead of at
the start of the second line:
Bad:
- PowerData = (1 << 31) | (0 << 30) | (24 << 24)
- | BitReverse(w89rf242_txvga_data[i][0], 24);
Good:
+ PowerData = (1 << 31) | (0 << 30) | (24 << 24) |
+ BitReverse(w89rf242_txvga_data[i][0], 24);
Writing the Changelog
Use the word "cleanup" instead of "fix". "Fix" implies the runtime changed and
it fixes a bug. "Cleanup" implies that runtime stayed exactly the same.
On Mon, Sep 23, 2013 at 11:01 AM, Dan Carpenter
<[email protected]> wrote:
> I've written a checkpatch guide for newbies because it seems like they
> make the same mistakes over and over. I intend to put it under
> Documentation/. Could you look it over?
>
>
> Introduction
>
> This document is aimed at new kernel contributors using "checkpatch.pl --file".
>
> The first thing to remember is that the aim is not to get rid of every
> checkpatch.pl warning; the aim is to make the code cleaner and more readable.
> The other thing to remember is that checkpatch.pl is not a very smart tool and
> there are mistakes that it misses so keep your eyes open for those as well.
>
> For example, checkpatch.pl could warn about a badly formatted warning message.
> Ask yourself, is the warning message is clear? Is it needed? Could a
> non-privileged user trigger the warning and flood the syslog? Are there
> spelling mistakes? Since Checkpatch.pl has flagged the line as sloppy code,
> there may be multiple mistakes.
>
> In the Linux kernel, we take an enormous pride in our work and we want clean
> code. But the one major drawback to cleanup patches is that they break
> "git blame" so it's not a good idea for newbies to send very trivial cleanup
> patches to the kernel/ directory. It's better to get a little experience in the
> drivers/ directory first. The drivers/staging/ directory in particular always
> needs cleanup patches.
>
>
> General Hints
>
> 1) Don't put too many things in one patch because it makes it hard to review.
> Break the patch up into a patch series like this made up example:
>
> [PATCH 1/4] subsystem: driver: Use tabs to indent instead of spaces
> [PATCH 2/4] subsystem: driver: Add spaces around math operations
> [PATCH 3/4] subsystem: driver: Remove extra braces
> [PATCH 4/4] subsystem: driver: Delete LINUX_VERSION_CODE related code
>
>
> Long Lines
>
> Historically screens were 80 characters wide and it was annoying when code went
> over the edge. These days we have larger screens, but we keep the 80 character
> limit because it forces us to write simpler code.
>
> One way to remove indent levels is using functions. If you find yourself
> writing a loop or a switch statement and you're already indented several tabs
> then probably it should be a new function instead.
>
> Whenever possible return immediately.
> Bad:
> - foo = kmalloc();
> - if (foo) {
> - /* code indent 2 tabs */
> - }
> - return foo;
> Good:
> + foo = kmalloc();
> + if (!foo)
> + return NULL;
> + /* code indented 1 tab */
> + return foo;
>
> Choose shorter names.
> Bad:
> - for(uiIpv6AddIndex = 0; uiIpv6AddIndex < uiIpv6AddrNoLongWords;
> - uiIpv6AddIndex++) {
> Good:
> + for (i = 0; i < long_words; i++) {
>
> Use temporary variable names:
> Bad:
> - dev->backdev[count]->bitlistsize =
> - dev->backdev[count]->devmagic->bitlistsize;
> Good:
> + back = dev->backdev[count];
> + back->bitlistsize = back->devmagic->bitlistsize;
>
> Don't do complicated things in the initializer:
> Bad:
> - struct binder_ref_death *tmp_death = container_of(w,
> - struct binder_ref_death, work);
> Good:
> + struct binder_ref_death *tmp_death;
> +
> + tmp_death = container_of(w, struct binder_ref_death, work);
>
> If you must break up a long line then align it nicely. Use spaces if needed.
> Bad:
> - if (adapter->flowcontrol == FLOW_RXONLY ||
> - adapter->flowcontrol == FLOW_BOTH) {
> Good:
> + if (adapter->flowcontrol == FLOW_RXONLY ||
> + adapter->flowcontrol == FLOW_BOTH) {
I needed this yesterday. I was not comfortable using spaces and I did
not know where the second line should begin.
>
> It's preferred if the operator goes at the end of the first line instead of at
> the start of the second line:
> Bad:
> - PowerData = (1 << 31) | (0 << 30) | (24 << 24)
> - | BitReverse(w89rf242_txvga_data[i][0], 24);
> Good:
> + PowerData = (1 << 31) | (0 << 30) | (24 << 24) |
> + BitReverse(w89rf242_txvga_data[i][0], 24);
What is the rule for where to start the second line here?
>
>
> Writing the Changelog
>
> Use the word "cleanup" instead of "fix". "Fix" implies the runtime changed and
> it fixes a bug. "Cleanup" implies that runtime stayed exactly the same.
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Peter
On Mon, Sep 23, 2013 at 02:46:38PM +0200, Peter Senna Tschudin wrote:
> > It's preferred if the operator goes at the end of the first line instead of at
> > the start of the second line:
> > Bad:
> > - PowerData = (1 << 31) | (0 << 30) | (24 << 24)
> > - | BitReverse(w89rf242_txvga_data[i][0], 24);
> > Good:
> > + PowerData = (1 << 31) | (0 << 30) | (24 << 24) |
> > + BitReverse(w89rf242_txvga_data[i][0], 24);
> What is the rule for where to start the second line here?
>
The if statement alignment has become a rule and people will make you
redo it if it's not aligned. I haven't seen anyone have to redo a
patch because of alignment on these:
My favorite format is aligned:
foo = xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx -
yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy;
Also popular:
foo = xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx -
yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy;
Right aligned looks like nonsense:
foo = xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx -
yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy;
Here is a more complex aligned statement:
foo = bar * (xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx -
yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy);
regards,
dan carpenter
On Mon, Sep 23, 2013 at 2:46 PM, Peter Senna Tschudin
<[email protected]> wrote:
>> Ask yourself, is the warning message is clear? Is it needed? Could a
s/is clear/clear/
>> It's preferred if the operator goes at the end of the first line instead of at
>> the start of the second line:
>> Bad:
>> - PowerData = (1 << 31) | (0 << 30) | (24 << 24)
>> - | BitReverse(w89rf242_txvga_data[i][0], 24);
>> Good:
>> + PowerData = (1 << 31) | (0 << 30) | (24 << 24) |
>> + BitReverse(w89rf242_txvga_data[i][0], 24);
> What is the rule for where to start the second line here?
Either aligned to the equivalent part on the previous line (i.e. "BitReverse" is
aligned with "(1 << 31)"), or one indent step more than the beginning of the
first line (i.e. "BitReverse" is indented by one more TAB tjan "PowerData").
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Mon, 2013-09-23 at 12:01 +0300, Dan Carpenter wrote:
> I've written a checkpatch guide for newbies because it seems like they
> make the same mistakes over and over. I intend to put it under
> Documentation/. Could you look it over?
>
>
> Introduction
>
> This document is aimed at new kernel contributors using "checkpatch.pl --file".
>
> The first thing to remember is that the aim is not to get rid of every
> checkpatch.pl warning; the aim is to make the code cleaner and more readable.
> The other thing to remember is that checkpatch.pl is not a very smart tool and
> there are mistakes that it misses so keep your eyes open for those as well.
Maybe suggest using
checkpatch.pl --file --fix --strict --types=<specific_type> <file>
mv <file>.EXPERIMENTAL-checkpatch-fixes <file>
git diff <file>
where <specific_types> is one of
For whitespace only changes
SPACING
SPACE_BEFORE_TAB
POINTER_LOCATION
For code changes:
C99_COMMENTS
PREFER_PACKED
PREFER_ALIGNED
AVOID_EXTERNS
PARENTHESIS_ALIGNMENT
> For example, checkpatch.pl could warn about a badly formatted warning message.
> Ask yourself, is the warning message is clear? Is it needed? Could a
> non-privileged user trigger the warning and flood the syslog? Are there
> spelling mistakes? Since Checkpatch.pl has flagged the line as sloppy code,
> there may be multiple mistakes.
>
> In the Linux kernel, we take an enormous pride in our work and we want clean
> code. But the one major drawback to cleanup patches is that they break
> "git blame" so it's not a good idea for newbies to send very trivial cleanup
> patches to the kernel/ directory. It's better to get a little experience in the
> drivers/ directory first. The drivers/staging/ directory in particular always
> needs cleanup patches.
I think suggesting working _only_ in the drivers/staging/
directory tree might be better.
Maybe I'll submit some auto-neatening script eventually
and see what you think.
Hi Dan,
On Mon, Sep 23, 2013 at 11:01 AM, Dan Carpenter
<[email protected]> wrote:
> I've written a checkpatch guide for newbies because it seems like they
> make the same mistakes over and over. I intend to put it under
> Documentation/. Could you look it over?
Maybe to add a paragraph about general work flow with linux-next? Or
is it better to have separate guide for this.
Myself, I was not able to find a good newbie receipt for it, except [1].
[1]
http://lwn.net/Articles/289245/
Regards,
Bojan
On Mon, 2013-09-23 at 11:06 -0700, Joe Perches wrote:
> Maybe I'll submit some auto-neatening script eventually
> and see what you think.
Maybe something like:
From: Joe Perches <[email protected]>
Subject: [PATCH] scripts/fix_with_checkpatch.sh: Add a trivial script to fix simple style defects
Add a few whitespace and style corrections to individual files.
Create git commits for those changes too.
Signed-off-by: Joe Perches <[email protected]>
---
scripts/fix_with_checkpatch.sh | 52 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
create mode 100755 scripts/fix_with_checkpatch.sh
diff --git a/scripts/fix_with_checkpatch.sh b/scripts/fix_with_checkpatch.sh
new file mode 100755
index 0000000..c47b111b
--- /dev/null
+++ b/scripts/fix_with_checkpatch.sh
@@ -0,0 +1,50 @@
+#/bin/sh
+
+whitespace_types="spacing space_before_tab pointer_location"
+changecode_types="c99_comments prefer_packed prefer_aligned avoid_externs parenthesis_alignment"
+
+checkpatch_update ()
+{
+ local file="$1"
+ type="$2"
+
+ ./scripts/checkpatch.pl --file --fix --strict --types=$type $file
+ if [ ! -f $file.EXPERIMENTAL-checkpatch-fixes ] ; then
+ return;
+ fi
+
+ mv $file.EXPERIMENTAL-checkpatch-fixes $file
+
+ git diff --stat -p $file | cat
+
+ obj="$(echo $file|sed 's/\.c$/\.o/')"
+
+ make $obj
+
+ tmpfile=$(mktemp git_commit.XXXXXX)
+ echo "$file: checkpatch neatening for $2" > $tmpfile
+ echo "" >> $tmpfile
+ echo "" >> $tmpfile
+
+ git commit -s -F $tmpfile -e $file
+
+ rm -f $tmpfile
+}
+
+for file in "$@"
+do
+
+ if [ ! -f $file ] ; then
+ echo "Argument '$file' is not a file"
+ continue
+ fi
+
+ for type in $whitespace_types ; do
+ checkpatch_update $file $type
+ done
+
+ for type in $changecode_types ; do
+ checkpatch_update $file $type
+ done
+
+done
--
1.8.1.2.459.gbcd45b4.dirty
On Mon, Sep 23, 2013 at 3:01 AM, Dan Carpenter <[email protected]> wrote:
> I've written a checkpatch guide for newbies because it seems like they
> make the same mistakes over and over. I intend to put it under
> Documentation/. Could you look it over?
I think this is great.
> Introduction
>
> This document is aimed at new kernel contributors using "checkpatch.pl --file".
>
> The first thing to remember is that the aim is not to get rid of every
> checkpatch.pl warning; the aim is to make the code cleaner and more readable.
> The other thing to remember is that checkpatch.pl is not a very smart tool and
> there are mistakes that it misses so keep your eyes open for those as well.
>
> For example, checkpatch.pl could warn about a badly formatted warning message.
> Ask yourself, is the warning message is clear? Is it needed? Could a
> non-privileged user trigger the warning and flood the syslog? Are there
> spelling mistakes? Since Checkpatch.pl has flagged the line as sloppy code,
> there may be multiple mistakes.
>
> In the Linux kernel, we take an enormous pride in our work and we want clean
> code. But the one major drawback to cleanup patches is that they break
> "git blame" so it's not a good idea for newbies to send very trivial cleanup
> patches to the kernel/ directory. It's better to get a little experience in the
> drivers/ directory first. The drivers/staging/ directory in particular always
> needs cleanup patches.
>
>
> General Hints
>
> 1) Don't put too many things in one patch because it makes it hard to review.
> Break the patch up into a patch series like this made up example:
>
> [PATCH 1/4] subsystem: driver: Use tabs to indent instead of spaces
> [PATCH 2/4] subsystem: driver: Add spaces around math operations
> [PATCH 3/4] subsystem: driver: Remove extra braces
> [PATCH 4/4] subsystem: driver: Delete LINUX_VERSION_CODE related code
This is a good hint and a good example. One thing that annoys me is
when there's no consistency in subject lines; I wish people would run
"git log --oneline <file>" and follow the existing convention,
including spacing, brackets, capitalization, etc. I also like Ingo's
suggestion [1] for making them sentences, starting with a verb.
But maybe that's too nitty-gritty to include. You already have a nice
concise writeup and it would be a shame to let us ruin it by committee
:)
Bjorn
[1] http://lkml.kernel.org/r/[email protected]
> Long Lines
>
> Historically screens were 80 characters wide and it was annoying when code went
> over the edge. These days we have larger screens, but we keep the 80 character
> limit because it forces us to write simpler code.
>
> One way to remove indent levels is using functions. If you find yourself
> writing a loop or a switch statement and you're already indented several tabs
> then probably it should be a new function instead.
>
> Whenever possible return immediately.
> Bad:
> - foo = kmalloc();
> - if (foo) {
> - /* code indent 2 tabs */
> - }
> - return foo;
> Good:
> + foo = kmalloc();
> + if (!foo)
> + return NULL;
> + /* code indented 1 tab */
> + return foo;
>
> Choose shorter names.
> Bad:
> - for(uiIpv6AddIndex = 0; uiIpv6AddIndex < uiIpv6AddrNoLongWords;
> - uiIpv6AddIndex++) {
> Good:
> + for (i = 0; i < long_words; i++) {
>
> Use temporary variable names:
> Bad:
> - dev->backdev[count]->bitlistsize =
> - dev->backdev[count]->devmagic->bitlistsize;
> Good:
> + back = dev->backdev[count];
> + back->bitlistsize = back->devmagic->bitlistsize;
>
> Don't do complicated things in the initializer:
> Bad:
> - struct binder_ref_death *tmp_death = container_of(w,
> - struct binder_ref_death, work);
> Good:
> + struct binder_ref_death *tmp_death;
> +
> + tmp_death = container_of(w, struct binder_ref_death, work);
>
> If you must break up a long line then align it nicely. Use spaces if needed.
> Bad:
> - if (adapter->flowcontrol == FLOW_RXONLY ||
> - adapter->flowcontrol == FLOW_BOTH) {
> Good:
> + if (adapter->flowcontrol == FLOW_RXONLY ||
> + adapter->flowcontrol == FLOW_BOTH) {
>
> It's preferred if the operator goes at the end of the first line instead of at
> the start of the second line:
> Bad:
> - PowerData = (1 << 31) | (0 << 30) | (24 << 24)
> - | BitReverse(w89rf242_txvga_data[i][0], 24);
> Good:
> + PowerData = (1 << 31) | (0 << 30) | (24 << 24) |
> + BitReverse(w89rf242_txvga_data[i][0], 24);
>
>
> Writing the Changelog
>
> Use the word "cleanup" instead of "fix". "Fix" implies the runtime changed and
> it fixes a bug. "Cleanup" implies that runtime stayed exactly the same.
Am 24.09.2013 18:36, schrieb Bjorn Helgaas:
> On Mon, Sep 23, 2013 at 3:01 AM, Dan Carpenter <[email protected]> wrote:
>> Long Lines
>>
>> Historically screens were 80 characters wide and it was annoying when code went
>> over the edge. These days we have larger screens, but we keep the 80 character
>> limit because it forces us to write simpler code.
Sorry, but that just isn't true and never was. Having a line wide limit
of 80 characters while forcing tabs to be 8 characters long limits most
code to just 72 characters. And even less (max 64) inside constructs
like if, for or while.
The only outcome of that totally silly rule is that variable names will
become shorted to silly acronyms almost nobody does understand make code
unreadable.
I always feel like beeing in the IT stone age when programmers thought
they have to use variable names like a, b and c to save storage, memory
or to type less when reading linux kernel code.
Regards,
Alexander Holler
Am 24.09.2013 19:26, schrieb Alexander Holler:
> Am 24.09.2013 18:36, schrieb Bjorn Helgaas:
>> On Mon, Sep 23, 2013 at 3:01 AM, Dan Carpenter
>> <[email protected]> wrote:
>>> Long Lines
>>>
>>> Historically screens were 80 characters wide and it was annoying when
>>> code went
>>> over the edge. These days we have larger screens, but we keep the 80
>>> character
>>> limit because it forces us to write simpler code.
>
> Sorry, but that just isn't true and never was. Having a line wide limit
> of 80 characters while forcing tabs to be 8 characters long limits most
> code to just 72 characters. And even less (max 64) inside constructs
> like if, for or while.
>
> The only outcome of that totally silly rule is that variable names will
> become shorted to silly acronyms almost nobody does understand make code
> unreadable.
I forgot to mention function names, which are often even worse shortened
than variable names.
>
> I always feel like beeing in the IT stone age when programmers thought
> they have to use variable names like a, b and c to save storage, memory
> or to type less when reading linux kernel code.
>
> Regards,
>
> Alexander Holler
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Am 24.09.2013 19:43, schrieb Alexander Holler:
> Am 24.09.2013 19:26, schrieb Alexander Holler:
>> Am 24.09.2013 18:36, schrieb Bjorn Helgaas:
>>> On Mon, Sep 23, 2013 at 3:01 AM, Dan Carpenter
>>> <[email protected]> wrote:
>>>> Long Lines
>>>>
>>>> Historically screens were 80 characters wide and it was annoying when
>>>> code went
>>>> over the edge. These days we have larger screens, but we keep the 80
>>>> character
>>>> limit because it forces us to write simpler code.
>>
>> Sorry, but that just isn't true and never was. Having a line wide limit
>> of 80 characters while forcing tabs to be 8 characters long limits most
>> code to just 72 characters. And even less (max 64) inside constructs
>> like if, for or while.
>>
>> The only outcome of that totally silly rule is that variable names will
>> become shorted to silly acronyms almost nobody does understand make code
>> unreadable.
>
> I forgot to mention function names, which are often even worse shortened
> than variable names.
>
>>
>> I always feel like beeing in the IT stone age when programmers thought
>> they have to use variable names like a, b and c to save storage, memory
>> or to type less when reading linux kernel code.
Just in case of ...
It isn't my intention to start a discussion about that 80 character
limit, but justifying it with wrong arguments shouldn't be done.
You could write "'It's there and you have to live with it.", but trying
to justify it with obviously wrong arguments should not be done.
Code doesn't get more simple by forcing people to use akronymous,
abrevations or to split lines.
Regards,
Alexander Holler
On Tue, Sep 24, 2013 at 7:26 PM, Alexander Holler <[email protected]> wrote:
> Am 24.09.2013 18:36, schrieb Bjorn Helgaas:
>>
>> On Mon, Sep 23, 2013 at 3:01 AM, Dan Carpenter <[email protected]>
>> wrote:
>>>
>>> Long Lines
>>>
>>> Historically screens were 80 characters wide and it was annoying when
>>> code went
>>> over the edge. These days we have larger screens, but we keep the 80
>>> character
>>> limit because it forces us to write simpler code.
>
>
> Sorry, but that just isn't true and never was. Having a line wide limit of
> 80 characters while forcing tabs to be 8 characters long limits most code to
> just 72 characters. And even less (max 64) inside constructs like if, for or
> while.
>
> The only outcome of that totally silly rule is that variable names will
> become shorted to silly acronyms almost nobody does understand make code
> unreadable.
>
> I always feel like beeing in the IT stone age when programmers thought they
> have to use variable names like a, b and c to save storage, memory or to
> type less when reading linux kernel code.
I was about to disagree because I've never seen variables named a, b
or c, but I found that there are at least 2238 variables named a, b or
c in linux-next. This is not good.
>
> Regards,
>
> Alexander Holler
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors"
> in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Peter
On Tue, Sep 24, 2013 at 09:29:49PM +0200, Peter Senna Tschudin wrote:
> I was about to disagree because I've never seen variables named a, b
> or c, but I found that there are at least 2238 variables named a, b or
> c in linux-next. This is not good.
>
In XGIfb_mode_rate_to_ddata() we have:
int B, C, D, F, temp, j;
The A and E variables were removed when the code was refactored. ;P
Places like this are fairly rare in the kernel outside of the staging/
directory.
There are lots of times where a single letter variable name is very
natural.
char c;
C is terse. This is explained in Documentation/CodingStyle.
regards,
dan carpenter
On Tue, Sep 24, 2013 at 1:29 PM, Peter Senna Tschudin
<[email protected]> wrote:
> On Tue, Sep 24, 2013 at 7:26 PM, Alexander Holler <[email protected]> wrote:
>>> On Mon, Sep 23, 2013 at 3:01 AM, Dan Carpenter <[email protected]>
>>> wrote:
>>>>
>>>> Long Lines
>>>>
>>>> Historically screens were 80 characters wide and it was annoying when
>>>> code went
>>>> over the edge. These days we have larger screens, but we keep the 80
>>>> character
>>>> limit because it forces us to write simpler code.
>>
>> Sorry, but that just isn't true and never was. Having a line wide limit of
>> 80 characters while forcing tabs to be 8 characters long limits most code to
>> just 72 characters. And even less (max 64) inside constructs like if, for or
>> while.
>>
>> The only outcome of that totally silly rule is that variable names will
>> become shorted to silly acronyms almost nobody does understand make code
>> unreadable.
In the context of a two-sentence paragraph, Dan's original text is
pithy and accurate. A Wikipedia article would deserve more
elaboration.
Obviously the skill of the programmer is the overwhelming factor, but
I think restricting the line length does help encourage simpler,
better-factored code. It's also part of the whole "it's better to be
consistent than to be better" thing. If 95% of the files in Linux use
80-character lines and the remainder use longer lines, it's just an
ongoing hassle for the reader.
>> I always feel like beeing in the IT stone age when programmers thought they
>> have to use variable names like a, b and c to save storage, memory or to
>> type less when reading linux kernel code.
> I was about to disagree because I've never seen variables named a, b
> or c, but I found that there are at least 2238 variables named a, b or
> c in linux-next. This is not good.
That is not self-evident. In many cases, e.g., loop iterators, simple
names are fine. Nothing is gained by renaming a loop counter from "a"
to "array_index." Simple names for simple things help the reader
focus on more important aspects of the code.
Bjorn
Am 24.09.2013 22:13, schrieb Bjorn Helgaas:
> On Tue, Sep 24, 2013 at 1:29 PM, Peter Senna Tschudin
> <[email protected]> wrote:
>> On Tue, Sep 24, 2013 at 7:26 PM, Alexander Holler <[email protected]> wrote:
>>>> On Mon, Sep 23, 2013 at 3:01 AM, Dan Carpenter <[email protected]>
>>>> wrote:
>>>>>
>>>>> Long Lines
>>>>>
>>>>> Historically screens were 80 characters wide and it was annoying when
>>>>> code went
>>>>> over the edge. These days we have larger screens, but we keep the 80
>>>>> character
>>>>> limit because it forces us to write simpler code.
>>>
>>> Sorry, but that just isn't true and never was. Having a line wide limit of
>>> 80 characters while forcing tabs to be 8 characters long limits most code to
>>> just 72 characters. And even less (max 64) inside constructs like if, for or
>>> while.
>>>
>>> The only outcome of that totally silly rule is that variable names will
>>> become shorted to silly acronyms almost nobody does understand make code
>>> unreadable.
>
> In the context of a two-sentence paragraph, Dan's original text is
> pithy and accurate. A Wikipedia article would deserve more
> elaboration.
>
> Obviously the skill of the programmer is the overwhelming factor, but
> I think restricting the line length does help encourage simpler,
> better-factored code. It's also part of the whole "it's better to be
> consistent than to be better" thing. If 95% of the files in Linux use
> 80-character lines and the remainder use longer lines, it's just an
> ongoing hassle for the reader.
>
>>> I always feel like beeing in the IT stone age when programmers thought they
>>> have to use variable names like a, b and c to save storage, memory or to
>>> type less when reading linux kernel code.
>> I was about to disagree because I've never seen variables named a, b
>> or c, but I found that there are at least 2238 variables named a, b or
>> c in linux-next. This is not good.
>
> That is not self-evident. In many cases, e.g., loop iterators, simple
> names are fine. Nothing is gained by renaming a loop counter from "a"
> to "array_index." Simple names for simple things help the reader
> focus on more important aspects of the code.
Sure and I'm the last one who wants that people do have to use anything
else than i for simple loop counters. And allowing longer lines doesn't
mean people have to use long names, it allows them use them (if it makes
sense). That's a big difference.
On the other side it's almost impossible to use verbose variable or
function names where they would make sense. Not to speak about all the
ugly splitted lines just to be below that ancient CGA limit.
So such a limit doesn't enforces or helps people to write simpler code.
It encourages using silly names and confusing line breaks, but in no way
it helps in writing more simple code. It might make help to keep the
code base consistent in regard to line lengths and confusing line
breaks, but that's almost all it does.
Regards,
Alexander Holler
On Wed, Sep 25, 2013 at 12:10:57AM +0200, Alexander Holler wrote:
> Sure and I'm the last one who wants that people do have to use
> anything else than i for simple loop counters. And allowing longer
> lines doesn't mean people have to use long names, it allows them use
> them (if it makes sense). That's a big difference.
>
> On the other side it's almost impossible to use verbose variable or
> function names where they would make sense. Not to speak about all
> the ugly splitted lines just to be below that ancient CGA limit.
Yeah, the things people will do to avoid <gasp> not nesting the living
hell out of their code...
Tell you what - pick a random place where the code is nested 8 levels
deep and I'm fairly sure that you will end up with your finger on one
hell of ugliness. Ugliness in logical structure, not in forced line
breaks. Let's experiment... Aha. In drivers/pci/hotplug/ibmphp_pci.c,
258 lines of deeply indented shite^Wcode that must be good since it compiles:
<avert your eyes if you've got a weak stomach>
======================================================================
int ibmphp_configure_card (struct pci_func *func, u8 slotno)
{
u16 vendor_id;
u32 class;
u8 class_code;
u8 hdr_type, device, sec_number;
u8 function;
struct pci_func *newfunc; /* for multi devices */
struct pci_func *cur_func, *prev_func;
int rc, i, j;
int cleanup_count;
u8 flag;
u8 valid_device = 0x00; /* to see if we are able to read from card any device info at all */
debug ("inside configure_card, func->busno = %x\n", func->busno);
device = func->device;
cur_func = func;
/* We only get bus and device from IRQ routing table. So at this point,
* func->busno is correct, and func->device contains only device (at the 5
* highest bits)
*/
/* For every function on the card */
for (function = 0x00; function < 0x08; function++) {
unsigned int devfn = PCI_DEVFN(device, function);
ibmphp_pci_bus->number = cur_func->busno;
cur_func->function = function;
debug ("inside the loop, cur_func->busno = %x, cur_func->device = %x, cur_func->function = %x\n",
cur_func->busno, cur_func->device, cur_func->function);
pci_bus_read_config_word (ibmphp_pci_bus, devfn, PCI_VENDOR_ID, &vendor_id);
debug ("vendor_id is %x\n", vendor_id);
if (vendor_id != PCI_VENDOR_ID_NOTVALID) {
/* found correct device!!! */
debug ("found valid device, vendor_id = %x\n", vendor_id);
++valid_device;
/* header: x x x x x x x x
* | |___________|=> 1=PPB bridge, 0=normal device, 2=CardBus Bridge
* |_=> 0 = single function device, 1 = multi-function device
*/
pci_bus_read_config_byte (ibmphp_pci_bus, devfn, PCI_HEADER_TYPE, &hdr_type);
pci_bus_read_config_dword (ibmphp_pci_bus, devfn, PCI_CLASS_REVISION, &class);
class_code = class >> 24;
debug ("hrd_type = %x, class = %x, class_code %x\n", hdr_type, class, class_code);
class >>= 8; /* to take revision out, class = class.subclass.prog i/f */
if (class == PCI_CLASS_NOT_DEFINED_VGA) {
err ("The device %x is VGA compatible and as is not supported for hot plugging. "
"Please choose another device.\n", cur_func->device);
return -ENODEV;
} else if (class == PCI_CLASS_DISPLAY_VGA) {
err ("The device %x is not supported for hot plugging. "
"Please choose another device.\n", cur_func->device);
return -ENODEV;
}
switch (hdr_type) {
case PCI_HEADER_TYPE_NORMAL:
debug ("single device case.... vendor id = %x, hdr_type = %x, class = %x\n", vendor_id, hdr_type, class);
assign_alt_irq (cur_func, class_code);
if ((rc = configure_device (cur_func)) < 0) {
/* We need to do this in case some other BARs were properly inserted */
err ("was not able to configure devfunc %x on bus %x.\n",
cur_func->device, cur_func->busno);
cleanup_count = 6;
goto error;
}
cur_func->next = NULL;
function = 0x8;
break;
case PCI_HEADER_TYPE_MULTIDEVICE:
assign_alt_irq (cur_func, class_code);
if ((rc = configure_device (cur_func)) < 0) {
/* We need to do this in case some other BARs were properly inserted */
err ("was not able to configure devfunc %x on bus %x...bailing out\n",
cur_func->device, cur_func->busno);
cleanup_count = 6;
goto error;
}
newfunc = kzalloc(sizeof(*newfunc), GFP_KERNEL);
if (!newfunc) {
err ("out of system memory\n");
return -ENOMEM;
}
newfunc->busno = cur_func->busno;
newfunc->device = device;
for (j = 0; j < 4; j++)
newfunc->irq[j] = cur_func->irq[j];
cur_func->next = newfunc;
cur_func = newfunc;
break;
case PCI_HEADER_TYPE_MULTIBRIDGE:
class >>= 8;
if (class != PCI_CLASS_BRIDGE_PCI) {
err ("This %x is not PCI-to-PCI bridge, and as is not supported for hot-plugging. "
"Please insert another card.\n", cur_func->device);
return -ENODEV;
}
assign_alt_irq (cur_func, class_code);
rc = configure_bridge (&cur_func, slotno);
if (rc == -ENODEV) {
err ("You chose to insert Single Bridge, or nested bridges, this is not supported...\n");
err ("Bus %x, devfunc %x\n", cur_func->busno, cur_func->device);
return rc;
}
if (rc) {
/* We need to do this in case some other BARs were properly inserted */
err ("was not able to hot-add PPB properly.\n");
func->bus = 1; /* To indicate to the unconfigure function that this is a PPB */
cleanup_count = 2;
goto error;
}
pci_bus_read_config_byte (ibmphp_pci_bus, devfn, PCI_SECONDARY_BUS, &sec_number);
flag = 0;
for (i = 0; i < 32; i++) {
if (func->devices[i]) {
newfunc = kzalloc(sizeof(*newfunc), GFP_KERNEL);
if (!newfunc) {
err ("out of system memory\n");
return -ENOMEM;
}
newfunc->busno = sec_number;
newfunc->device = (u8) i;
for (j = 0; j < 4; j++)
newfunc->irq[j] = cur_func->irq[j];
if (flag) {
for (prev_func = cur_func; prev_func->next; prev_func = prev_func->next) ;
prev_func->next = newfunc;
} else
cur_func->next = newfunc;
rc = ibmphp_configure_card (newfunc, slotno);
/* This could only happen if kmalloc failed */
if (rc) {
/* We need to do this in case bridge itself got configured properly, but devices behind it failed */
func->bus = 1; /* To indicate to the unconfigure function that this is a PPB */
cleanup_count = 2;
goto error;
}
flag = 1;
}
}
newfunc = kzalloc(sizeof(*newfunc), GFP_KERNEL);
if (!newfunc) {
err ("out of system memory\n");
return -ENOMEM;
}
newfunc->busno = cur_func->busno;
newfunc->device = device;
for (j = 0; j < 4; j++)
newfunc->irq[j] = cur_func->irq[j];
for (prev_func = cur_func; prev_func->next; prev_func = prev_func->next) ;
prev_func->next = newfunc;
cur_func = newfunc;
break;
case PCI_HEADER_TYPE_BRIDGE:
class >>= 8;
debug ("class now is %x\n", class);
if (class != PCI_CLASS_BRIDGE_PCI) {
err ("This %x is not PCI-to-PCI bridge, and as is not supported for hot-plugging. "
"Please insert another card.\n", cur_func->device);
return -ENODEV;
}
assign_alt_irq (cur_func, class_code);
debug ("cur_func->busno b4 configure_bridge is %x\n", cur_func->busno);
rc = configure_bridge (&cur_func, slotno);
if (rc == -ENODEV) {
err ("You chose to insert Single Bridge, or nested bridges, this is not supported...\n");
err ("Bus %x, devfunc %x\n", cur_func->busno, cur_func->device);
return rc;
}
if (rc) {
/* We need to do this in case some other BARs were properly inserted */
func->bus = 1; /* To indicate to the unconfigure function that this is a PPB */
err ("was not able to hot-add PPB properly.\n");
cleanup_count = 2;
goto error;
}
debug ("cur_func->busno = %x, device = %x, function = %x\n",
cur_func->busno, device, function);
pci_bus_read_config_byte (ibmphp_pci_bus, devfn, PCI_SECONDARY_BUS, &sec_number);
debug ("after configuring bridge..., sec_number = %x\n", sec_number);
flag = 0;
for (i = 0; i < 32; i++) {
if (func->devices[i]) {
debug ("inside for loop, device is %x\n", i);
newfunc = kzalloc(sizeof(*newfunc), GFP_KERNEL);
if (!newfunc) {
err (" out of system memory\n");
return -ENOMEM;
}
newfunc->busno = sec_number;
newfunc->device = (u8) i;
for (j = 0; j < 4; j++)
newfunc->irq[j] = cur_func->irq[j];
if (flag) {
for (prev_func = cur_func; prev_func->next; prev_func = prev_func->next) ;
prev_func->next = newfunc;
} else
cur_func->next = newfunc;
rc = ibmphp_configure_card (newfunc, slotno);
/* Again, this case should not happen... For complete paranoia, will need to call remove_bus */
if (rc) {
/* We need to do this in case some other BARs were properly inserted */
func->bus = 1; /* To indicate to the unconfigure function that this is a PPB */
cleanup_count = 2;
goto error;
}
flag = 1;
}
}
function = 0x8;
break;
default:
err ("MAJOR PROBLEM!!!!, header type not supported? %x\n", hdr_type);
return -ENXIO;
break;
} /* end of switch */
} /* end of valid device */
} /* end of for */
if (!valid_device) {
err ("Cannot find any valid devices on the card. Or unable to read from card.\n");
return -ENODEV;
}
return 0;
error:
for (i = 0; i < cleanup_count; i++) {
if (cur_func->io[i]) {
ibmphp_remove_resource (cur_func->io[i]);
cur_func->io[i] = NULL;
} else if (cur_func->pfmem[i]) {
ibmphp_remove_resource (cur_func->pfmem[i]);
cur_func->pfmem[i] = NULL;
} else if (cur_func->mem[i]) {
ibmphp_remove_resource (cur_func->mem[i]);
cur_func->mem[i] = NULL;
}
}
return rc;
}
======================================================================
Note the lovely comments /* end of for */ et.al. - nice to see that somebody
understood that keeping track of that nesting might be not quite trivial.
Let's start with the obvious - we have a loop, with the end of the body
consisting of
if (vendor_id != PCI_VENDOR_ID_NOTVALID)
<197 lines of over-indented code>
And those lines are about 80% of the function. What could we use here?
'Sright, "continue". While we are at it, we have a couple of loops of
form
for (i = 0; i < 32; i++) {
if (func->devices[i]) {
<deeply nested shite>
}
}
The same cure, of course... Oh, and switch with long cases definitely
should not be indented that way. With all that dealt with, we get
still overindented and hard to read
======================================================================
int ibmphp_configure_card (struct pci_func *func, u8 slotno)
{
u16 vendor_id;
u32 class;
u8 class_code;
u8 hdr_type, device, sec_number;
u8 function;
struct pci_func *newfunc; /* for multi devices */
struct pci_func *cur_func, *prev_func;
int rc, i, j;
int cleanup_count;
u8 flag;
u8 valid_device = 0x00; /* to see if we are able to read from card any device info at all */
debug ("inside configure_card, func->busno = %x\n", func->busno);
device = func->device;
cur_func = func;
/* We only get bus and device from IRQ routing table. So at this point,
* func->busno is correct, and func->device contains only device (at the 5
* highest bits)
*/
/* For every function on the card */
for (function = 0x00; function < 0x08; function++) {
unsigned int devfn = PCI_DEVFN(device, function);
ibmphp_pci_bus->number = cur_func->busno;
cur_func->function = function;
debug ("inside the loop, cur_func->busno = %x, cur_func->device = %x, cur_func->function = %x\n",
cur_func->busno, cur_func->device, cur_func->function);
pci_bus_read_config_word (ibmphp_pci_bus, devfn, PCI_VENDOR_ID, &vendor_id);
debug ("vendor_id is %x\n", vendor_id);
if (vendor_id == PCI_VENDOR_ID_NOTVALID)
continue;
/* found correct device!!! */
debug ("found valid device, vendor_id = %x\n", vendor_id);
++valid_device;
/* header: x x x x x x x x
* | |___________|=> 1=PPB bridge, 0=normal device, 2=CardBus Bridge
* |_=> 0 = single function device, 1 = multi-function device
*/
pci_bus_read_config_byte (ibmphp_pci_bus, devfn, PCI_HEADER_TYPE, &hdr_type);
pci_bus_read_config_dword (ibmphp_pci_bus, devfn, PCI_CLASS_REVISION, &class);
class_code = class >> 24;
debug ("hrd_type = %x, class = %x, class_code %x\n", hdr_type, class, class_code);
class >>= 8; /* to take revision out, class = class.subclass.prog i/f */
if (class == PCI_CLASS_NOT_DEFINED_VGA) {
err ("The device %x is VGA compatible and as is not supported for hot plugging. "
"Please choose another device.\n", cur_func->device);
return -ENODEV;
} else if (class == PCI_CLASS_DISPLAY_VGA) {
err ("The device %x is not supported for hot plugging. "
"Please choose another device.\n", cur_func->device);
return -ENODEV;
}
switch (hdr_type) {
case PCI_HEADER_TYPE_NORMAL:
debug ("single device case.... vendor id = %x, hdr_type = %x, class = %x\n", vendor_id, hdr_type, class);
assign_alt_irq (cur_func, class_code);
if ((rc = configure_device (cur_func)) < 0) {
/* We need to do this in case some other BARs were properly inserted */
err ("was not able to configure devfunc %x on bus %x.\n",
cur_func->device, cur_func->busno);
cleanup_count = 6;
goto error;
}
cur_func->next = NULL;
function = 0x8;
break;
case PCI_HEADER_TYPE_MULTIDEVICE:
assign_alt_irq (cur_func, class_code);
if ((rc = configure_device (cur_func)) < 0) {
/* We need to do this in case some other BARs were properly inserted */
err ("was not able to configure devfunc %x on bus %x...bailing out\n",
cur_func->device, cur_func->busno);
cleanup_count = 6;
goto error;
}
newfunc = kzalloc(sizeof(*newfunc), GFP_KERNEL);
if (!newfunc) {
err ("out of system memory\n");
return -ENOMEM;
}
newfunc->busno = cur_func->busno;
newfunc->device = device;
cur_func->next = newfunc;
cur_func = newfunc;
for (j = 0; j < 4; j++)
newfunc->irq[j] = cur_func->irq[j];
break;
case PCI_HEADER_TYPE_MULTIBRIDGE:
class >>= 8;
if (class != PCI_CLASS_BRIDGE_PCI) {
err ("This %x is not PCI-to-PCI bridge, and as is not supported for hot-plugging. "
"Please insert another card.\n", cur_func->device);
return -ENODEV;
}
assign_alt_irq (cur_func, class_code);
rc = configure_bridge (&cur_func, slotno);
if (rc == -ENODEV) {
err ("You chose to insert Single Bridge, or nested bridges, this is not supported...\n");
err ("Bus %x, devfunc %x\n", cur_func->busno, cur_func->device);
return rc;
}
if (rc) {
/* We need to do this in case some other BARs were properly inserted */
err ("was not able to hot-add PPB properly.\n");
func->bus = 1; /* To indicate to the unconfigure function that this is a PPB */
cleanup_count = 2;
goto error;
}
pci_bus_read_config_byte (ibmphp_pci_bus, devfn, PCI_SECONDARY_BUS, &sec_number);
flag = 0;
for (i = 0; i < 32; i++) {
if (!func->devices[i])
continue;
newfunc = kzalloc(sizeof(*newfunc), GFP_KERNEL);
if (!newfunc) {
err ("out of system memory\n");
return -ENOMEM;
}
newfunc->busno = sec_number;
newfunc->device = (u8) i;
for (j = 0; j < 4; j++)
newfunc->irq[j] = cur_func->irq[j];
if (flag) {
for (prev_func = cur_func; prev_func->next; prev_func = prev_func->next) ;
prev_func->next = newfunc;
} else
cur_func->next = newfunc;
rc = ibmphp_configure_card (newfunc, slotno);
/* This could only happen if kmalloc failed */
if (rc) {
/* We need to do this in case bridge itself got configured properly, but devices behind it failed */
func->bus = 1; /* To indicate to the unconfigure function that this is a PPB */
cleanup_count = 2;
goto error;
}
flag = 1;
}
newfunc = kzalloc(sizeof(*newfunc), GFP_KERNEL);
if (!newfunc) {
err ("out of system memory\n");
return -ENOMEM;
}
newfunc->busno = cur_func->busno;
newfunc->device = device;
for (j = 0; j < 4; j++)
newfunc->irq[j] = cur_func->irq[j];
for (prev_func = cur_func; prev_func->next; prev_func = prev_func->next) ;
prev_func->next = newfunc;
cur_func = newfunc;
break;
case PCI_HEADER_TYPE_BRIDGE:
class >>= 8;
debug ("class now is %x\n", class);
if (class != PCI_CLASS_BRIDGE_PCI) {
err ("This %x is not PCI-to-PCI bridge, and as is not supported for hot-plugging. "
"Please insert another card.\n", cur_func->device);
return -ENODEV;
}
assign_alt_irq (cur_func, class_code);
debug ("cur_func->busno b4 configure_bridge is %x\n", cur_func->busno);
rc = configure_bridge (&cur_func, slotno);
if (rc == -ENODEV) {
err ("You chose to insert Single Bridge, or nested bridges, this is not supported...\n");
err ("Bus %x, devfunc %x\n", cur_func->busno, cur_func->device);
return rc;
}
if (rc) {
/* We need to do this in case some other BARs were properly inserted */
func->bus = 1; /* To indicate to the unconfigure function that this is a PPB */
err ("was not able to hot-add PPB properly.\n");
cleanup_count = 2;
goto error;
}
debug ("cur_func->busno = %x, device = %x, function = %x\n",
cur_func->busno, device, function);
pci_bus_read_config_byte (ibmphp_pci_bus, devfn, PCI_SECONDARY_BUS, &sec_number);
debug ("after configuring bridge..., sec_number = %x\n", sec_number);
flag = 0;
for (i = 0; i < 32; i++) {
if (!func->devices[i])
continue;
debug ("inside for loop, device is %x\n", i);
newfunc = kzalloc(sizeof(*newfunc), GFP_KERNEL);
if (!newfunc) {
err (" out of system memory\n");
return -ENOMEM;
}
newfunc->busno = sec_number;
newfunc->device = (u8) i;
for (j = 0; j < 4; j++)
newfunc->irq[j] = cur_func->irq[j];
if (flag) {
for (prev_func = cur_func; prev_func->next; prev_func = prev_func->next) ;
prev_func->next = newfunc;
} else
cur_func->next = newfunc;
rc = ibmphp_configure_card (newfunc, slotno);
/* Again, this case should not happen... For complete paranoia, will need to call remove_bus */
if (rc) {
/* We need to do this in case some other BARs were properly inserted */
func->bus = 1; /* To indicate to the unconfigure function that this is a PPB */
cleanup_count = 2;
goto error;
}
flag = 1;
}
function = 0x8;
break;
default:
err ("MAJOR PROBLEM!!!!, header type not supported? %x\n", hdr_type);
return -ENXIO;
break;
} /* end of switch */
} /* end of for */
if (!valid_device) {
err ("Cannot find any valid devices on the card. Or unable to read from card.\n");
return -ENODEV;
}
return 0;
error:
for (i = 0; i < cleanup_count; i++) {
if (cur_func->io[i]) {
ibmphp_remove_resource (cur_func->io[i]);
cur_func->io[i] = NULL;
} else if (cur_func->pfmem[i]) {
ibmphp_remove_resource (cur_func->pfmem[i]);
cur_func->pfmem[i] = NULL;
} else if (cur_func->mem[i]) {
ibmphp_remove_resource (cur_func->mem[i]);
cur_func->mem[i] = NULL;
}
}
return rc;
}
======================================================================
What's really deeply indented here? Right,
if (flag) {
for (prev_func = cur_func; prev_func->next; prev_func = prev_func->next) ;
prev_func->next = newfunc;
} else
cur_func->next = newfunc;
Just WTF is going on here? Looks like conditional insertion into the end of
list, but what the hell is "flag"? Further digging shows that it starts as
0 and gets set to 1 after the first pass through that...
<looks for places assigning cur_func>
Egads...
cur_func = newfunc;
for (j = 0; j < 4; j++)
newfunc->irq[j] = cur_func->irq[j];
OK, that's our first bug right there...
Anyway, can cur_func->next be non-NULL when flag is 0? It can't be non-NULL
when we enter this function (both for the initial caller and for recursive
ones); func has been freshly kzalloc'ed in all those cases, with ->next
not modified in the meanwhile. configure_device() doesn't change ->next.
That fragment reassigning cur_func has its ->next equal to NULL (again,
kzalloc()). configure_bridge()... WTF? It gets &cur_func passed to it,
then it has
func = *func_passed;
/* a lot of code that never modifies func */
func_passed = &func;
debug ("func->busno b4 returning is %x\n", func->busno);
debug ("func->busno b4 returning in the other structure is %x\n", (*func_passed)->busno);
kfree (amount_needed);
WHAT other structure? After that func_passed = &func we will have *func_passed
equal to func, no matter what. How much crack had it taken to write that code,
anyway? Oh, well... func->next is not touched at all in there. Another
reassignment:
for (j = 0; j < 4; j++)
newfunc->irq[j] = cur_func->irq[j];
for (prev_func = cur_func; prev_func->next; prev_func = prev_func->next) ;
prev_func->next = newfunc;
cur_func = newfunc;
break;
case PCI_HEADER_TYPE_BRIDGE:
Again, ->next is NULL here, due to kzalloc().
Grr... OK, the right way to look at that: in the beginning of the outer
loop cur_func->next is NULL due to what callers have done; after each pass
through the loop we either bugger off (return or manually setting loop
index to upper limit) or we get cur_func->next guaranteed to be NULL again.
We never modify it in the body prior to entering switch. Ergo, it's NULL
on the entry to each case. We also do not modify it between case ...:
and flag = 0 inside the cases that have that shite.
In other words, all this "flag" business is pure obfuscation; what we are
doing here is appending to the end of list no matter what. OK, let's
take that to a helper function; proper fix is probably to keep track of
the _last_ element of the list through all that. Recursive calls will
have to report it to caller, but that's not hard to do. Separate story,
anyway.
OK, with aforementioned bug fixed that becomes
======================================================================
static void append(struct pci_func *head, struct pci_func *new)
{
while (head->next)
head = head->next;
head->next = new;
}
int ibmphp_configure_card (struct pci_func *func, u8 slotno)
{
u16 vendor_id;
u32 class;
u8 class_code;
u8 hdr_type, device, sec_number;
u8 function;
struct pci_func *newfunc; /* for multi devices */
struct pci_func *cur_func;
int rc, i, j;
int cleanup_count;
u8 valid_device = 0x00; /* to see if we are able to read from card any device info at all */
debug ("inside configure_card, func->busno = %x\n", func->busno);
device = func->device;
cur_func = func;
/* We only get bus and device from IRQ routing table. So at this point,
* func->busno is correct, and func->device contains only device (at the 5
* highest bits)
*/
/* For every function on the card */
for (function = 0x00; function < 0x08; function++) {
unsigned int devfn = PCI_DEVFN(device, function);
ibmphp_pci_bus->number = cur_func->busno;
cur_func->function = function;
debug ("inside the loop, cur_func->busno = %x, cur_func->device = %x, cur_func->function = %x\n",
cur_func->busno, cur_func->device, cur_func->function);
pci_bus_read_config_word (ibmphp_pci_bus, devfn, PCI_VENDOR_ID, &vendor_id);
debug ("vendor_id is %x\n", vendor_id);
if (vendor_id == PCI_VENDOR_ID_NOTVALID)
continue;
/* found correct device!!! */
debug ("found valid device, vendor_id = %x\n", vendor_id);
++valid_device;
/* header: x x x x x x x x
* | |___________|=> 1=PPB bridge, 0=normal device, 2=CardBus Bridge
* |_=> 0 = single function device, 1 = multi-function device
*/
pci_bus_read_config_byte (ibmphp_pci_bus, devfn, PCI_HEADER_TYPE, &hdr_type);
pci_bus_read_config_dword (ibmphp_pci_bus, devfn, PCI_CLASS_REVISION, &class);
class_code = class >> 24;
debug ("hrd_type = %x, class = %x, class_code %x\n", hdr_type, class, class_code);
class >>= 8; /* to take revision out, class = class.subclass.prog i/f */
if (class == PCI_CLASS_NOT_DEFINED_VGA) {
err ("The device %x is VGA compatible and as is not supported for hot plugging. "
"Please choose another device.\n", cur_func->device);
return -ENODEV;
} else if (class == PCI_CLASS_DISPLAY_VGA) {
err ("The device %x is not supported for hot plugging. "
"Please choose another device.\n", cur_func->device);
return -ENODEV;
}
switch (hdr_type) {
case PCI_HEADER_TYPE_NORMAL:
debug ("single device case.... vendor id = %x, hdr_type = %x, class = %x\n", vendor_id, hdr_type, class);
assign_alt_irq (cur_func, class_code);
if ((rc = configure_device (cur_func)) < 0) {
/* We need to do this in case some other BARs were properly inserted */
err ("was not able to configure devfunc %x on bus %x.\n",
cur_func->device, cur_func->busno);
cleanup_count = 6;
goto error;
}
cur_func->next = NULL;
function = 0x8;
break;
case PCI_HEADER_TYPE_MULTIDEVICE:
assign_alt_irq (cur_func, class_code);
if ((rc = configure_device (cur_func)) < 0) {
/* We need to do this in case some other BARs were properly inserted */
err ("was not able to configure devfunc %x on bus %x...bailing out\n",
cur_func->device, cur_func->busno);
cleanup_count = 6;
goto error;
}
newfunc = kzalloc(sizeof(*newfunc), GFP_KERNEL);
if (!newfunc) {
err ("out of system memory\n");
return -ENOMEM;
}
newfunc->busno = cur_func->busno;
newfunc->device = device;
cur_func->next = newfunc;
cur_func = newfunc;
for (j = 0; j < 4; j++)
newfunc->irq[j] = cur_func->irq[j];
break;
case PCI_HEADER_TYPE_MULTIBRIDGE:
class >>= 8;
if (class != PCI_CLASS_BRIDGE_PCI) {
err ("This %x is not PCI-to-PCI bridge, and as is not supported for hot-plugging. "
"Please insert another card.\n", cur_func->device);
return -ENODEV;
}
assign_alt_irq (cur_func, class_code);
rc = configure_bridge (&cur_func, slotno);
if (rc == -ENODEV) {
err ("You chose to insert Single Bridge, or nested bridges, this is not supported...\n");
err ("Bus %x, devfunc %x\n", cur_func->busno, cur_func->device);
return rc;
}
if (rc) {
/* We need to do this in case some other BARs were properly inserted */
err ("was not able to hot-add PPB properly.\n");
func->bus = 1; /* To indicate to the unconfigure function that this is a PPB */
cleanup_count = 2;
goto error;
}
pci_bus_read_config_byte (ibmphp_pci_bus, devfn, PCI_SECONDARY_BUS, &sec_number);
for (i = 0; i < 32; i++) {
if (!func->devices[i])
continue;
newfunc = kzalloc(sizeof(*newfunc), GFP_KERNEL);
if (!newfunc) {
err ("out of system memory\n");
return -ENOMEM;
}
newfunc->busno = sec_number;
newfunc->device = (u8) i;
for (j = 0; j < 4; j++)
newfunc->irq[j] = cur_func->irq[j];
append(cur_func, newfunc);
rc = ibmphp_configure_card (newfunc, slotno);
/* This could only happen if kmalloc failed */
if (rc) {
/* We need to do this in case bridge itself got configured properly, but devices behind it failed */
func->bus = 1; /* To indicate to the unconfigure function that this is a PPB */
cleanup_count = 2;
goto error;
}
}
newfunc = kzalloc(sizeof(*newfunc), GFP_KERNEL);
if (!newfunc) {
err ("out of system memory\n");
return -ENOMEM;
}
newfunc->busno = cur_func->busno;
newfunc->device = device;
for (j = 0; j < 4; j++)
newfunc->irq[j] = cur_func->irq[j];
append(cur_func, newfunc);
cur_func = newfunc;
break;
case PCI_HEADER_TYPE_BRIDGE:
class >>= 8;
debug ("class now is %x\n", class);
if (class != PCI_CLASS_BRIDGE_PCI) {
err ("This %x is not PCI-to-PCI bridge, and as is not supported for hot-plugging. "
"Please insert another card.\n", cur_func->device);
return -ENODEV;
}
assign_alt_irq (cur_func, class_code);
debug ("cur_func->busno b4 configure_bridge is %x\n", cur_func->busno);
rc = configure_bridge (&cur_func, slotno);
if (rc == -ENODEV) {
err ("You chose to insert Single Bridge, or nested bridges, this is not supported...\n");
err ("Bus %x, devfunc %x\n", cur_func->busno, cur_func->device);
return rc;
}
if (rc) {
/* We need to do this in case some other BARs were properly inserted */
func->bus = 1; /* To indicate to the unconfigure function that this is a PPB */
err ("was not able to hot-add PPB properly.\n");
cleanup_count = 2;
goto error;
}
debug ("cur_func->busno = %x, device = %x, function = %x\n",
cur_func->busno, device, function);
pci_bus_read_config_byte (ibmphp_pci_bus, devfn, PCI_SECONDARY_BUS, &sec_number);
debug ("after configuring bridge..., sec_number = %x\n", sec_number);
for (i = 0; i < 32; i++) {
if (!func->devices[i])
continue;
debug ("inside for loop, device is %x\n", i);
newfunc = kzalloc(sizeof(*newfunc), GFP_KERNEL);
if (!newfunc) {
err (" out of system memory\n");
return -ENOMEM;
}
newfunc->busno = sec_number;
newfunc->device = (u8) i;
for (j = 0; j < 4; j++)
newfunc->irq[j] = cur_func->irq[j];
append(cur_func, newfunc);
rc = ibmphp_configure_card (newfunc, slotno);
/* Again, this case should not happen... For complete paranoia, will need to call remove_bus */
if (rc) {
/* We need to do this in case some other BARs were properly inserted */
func->bus = 1; /* To indicate to the unconfigure function that this is a PPB */
cleanup_count = 2;
goto error;
}
}
function = 0x8;
break;
default:
err ("MAJOR PROBLEM!!!!, header type not supported? %x\n", hdr_type);
return -ENXIO;
break;
} /* end of switch */
} /* end of for */
if (!valid_device) {
err ("Cannot find any valid devices on the card. Or unable to read from card.\n");
return -ENODEV;
}
return 0;
error:
for (i = 0; i < cleanup_count; i++) {
if (cur_func->io[i]) {
ibmphp_remove_resource (cur_func->io[i]);
cur_func->io[i] = NULL;
} else if (cur_func->pfmem[i]) {
ibmphp_remove_resource (cur_func->pfmem[i]);
cur_func->pfmem[i] = NULL;
} else if (cur_func->mem[i]) {
ibmphp_remove_resource (cur_func->mem[i]);
cur_func->mem[i] = NULL;
}
}
return rc;
}
======================================================================
Now, a look at the places where we are appending suggests that it might
be worth folding kzalloc + setting busno/device/irq into append(), making
it return the new pci_func or NULL in case of failure. With that the
sucker starts looking as below and I'm quite certain that with a bit of
further massage it can be cleaned up more (I'd probably start with
moving local variables into the minimal blocks needing those and then
looked where it gets us). I think that my point is made, though - deeply
nested code is a very good predictor of shitty code. Not guaranteed,
of course, but as a Bayesian filter it works very well.
======================================================================
static struct pci_func *append(struct pci_func *head, u8 busno, u8 device)
{
struct pci_func *new = kzalloc(sizeof(*new), GFP_KERNEL);
int i;
if (!new) {
err ("out of system memory\n");
return NULL;
}
new->busno = busno;
new->device = device;
for (i = 0; i < 4; i++)
new->irq[i] = head->irq[i];
while (head->next)
head = head->next;
head->next = new;
return new;
}
int ibmphp_configure_card (struct pci_func *func, u8 slotno)
{
u16 vendor_id;
u32 class;
u8 class_code;
u8 hdr_type, device, sec_number;
u8 function;
struct pci_func *newfunc; /* for multi devices */
struct pci_func *cur_func;
int rc, i, j;
int cleanup_count;
u8 valid_device = 0x00; /* to see if we are able to read from card any device info at all */
debug ("inside configure_card, func->busno = %x\n", func->busno);
device = func->device;
cur_func = func;
/* We only get bus and device from IRQ routing table. So at this point,
* func->busno is correct, and func->device contains only device (at the 5
* highest bits)
*/
/* For every function on the card */
for (function = 0x00; function < 0x08; function++) {
unsigned int devfn = PCI_DEVFN(device, function);
ibmphp_pci_bus->number = cur_func->busno;
cur_func->function = function;
debug ("inside the loop, cur_func->busno = %x, cur_func->device = %x, cur_func->function = %x\n",
cur_func->busno, cur_func->device, cur_func->function);
pci_bus_read_config_word (ibmphp_pci_bus, devfn, PCI_VENDOR_ID, &vendor_id);
debug ("vendor_id is %x\n", vendor_id);
if (vendor_id == PCI_VENDOR_ID_NOTVALID)
continue;
/* found correct device!!! */
debug ("found valid device, vendor_id = %x\n", vendor_id);
++valid_device;
/* header: x x x x x x x x
* | |___________|=> 1=PPB bridge, 0=normal device, 2=CardBus Bridge
* |_=> 0 = single function device, 1 = multi-function device
*/
pci_bus_read_config_byte (ibmphp_pci_bus, devfn, PCI_HEADER_TYPE, &hdr_type);
pci_bus_read_config_dword (ibmphp_pci_bus, devfn, PCI_CLASS_REVISION, &class);
class_code = class >> 24;
debug ("hrd_type = %x, class = %x, class_code %x\n", hdr_type, class, class_code);
class >>= 8; /* to take revision out, class = class.subclass.prog i/f */
if (class == PCI_CLASS_NOT_DEFINED_VGA) {
err ("The device %x is VGA compatible and as is not supported for hot plugging. "
"Please choose another device.\n", cur_func->device);
return -ENODEV;
} else if (class == PCI_CLASS_DISPLAY_VGA) {
err ("The device %x is not supported for hot plugging. "
"Please choose another device.\n", cur_func->device);
return -ENODEV;
}
switch (hdr_type) {
case PCI_HEADER_TYPE_NORMAL:
debug ("single device case.... vendor id = %x, hdr_type = %x, class = %x\n", vendor_id, hdr_type, class);
assign_alt_irq (cur_func, class_code);
if ((rc = configure_device (cur_func)) < 0) {
/* We need to do this in case some other BARs were properly inserted */
err ("was not able to configure devfunc %x on bus %x.\n",
cur_func->device, cur_func->busno);
cleanup_count = 6;
goto error;
}
cur_func->next = NULL;
function = 0x8;
break;
case PCI_HEADER_TYPE_MULTIDEVICE:
assign_alt_irq (cur_func, class_code);
if ((rc = configure_device (cur_func)) < 0) {
/* We need to do this in case some other BARs were properly inserted */
err ("was not able to configure devfunc %x on bus %x...bailing out\n",
cur_func->device, cur_func->busno);
cleanup_count = 6;
goto error;
}
cur_func = append(cur_func, cur_func->busno, device);
if (!cur_func)
return -ENOMEM;
break;
case PCI_HEADER_TYPE_MULTIBRIDGE:
class >>= 8;
if (class != PCI_CLASS_BRIDGE_PCI) {
err ("This %x is not PCI-to-PCI bridge, and as is not supported for hot-plugging. "
"Please insert another card.\n", cur_func->device);
return -ENODEV;
}
assign_alt_irq (cur_func, class_code);
rc = configure_bridge (&cur_func, slotno);
if (rc == -ENODEV) {
err ("You chose to insert Single Bridge, or nested bridges, this is not supported...\n");
err ("Bus %x, devfunc %x\n", cur_func->busno, cur_func->device);
return rc;
}
if (rc) {
/* We need to do this in case some other BARs were properly inserted */
err ("was not able to hot-add PPB properly.\n");
func->bus = 1; /* To indicate to the unconfigure function that this is a PPB */
cleanup_count = 2;
goto error;
}
pci_bus_read_config_byte (ibmphp_pci_bus, devfn, PCI_SECONDARY_BUS, &sec_number);
for (i = 0; i < 32; i++) {
if (!func->devices[i])
continue;
newfunc = append(cur_func, sec_number, i);
if (!newfunc)
return -ENOMEM;
rc = ibmphp_configure_card (newfunc, slotno);
/* This could only happen if kmalloc failed */
if (rc) {
/* We need to do this in case bridge itself got configured properly, but devices behind it failed */
func->bus = 1; /* To indicate to the unconfigure function that this is a PPB */
cleanup_count = 2;
goto error;
}
}
cur_func = append(cur_func, cur_func->busno, device);
if (!cur_func)
return -ENOMEM;
break;
case PCI_HEADER_TYPE_BRIDGE:
class >>= 8;
debug ("class now is %x\n", class);
if (class != PCI_CLASS_BRIDGE_PCI) {
err ("This %x is not PCI-to-PCI bridge, and as is not supported for hot-plugging. "
"Please insert another card.\n", cur_func->device);
return -ENODEV;
}
assign_alt_irq (cur_func, class_code);
debug ("cur_func->busno b4 configure_bridge is %x\n", cur_func->busno);
rc = configure_bridge (&cur_func, slotno);
if (rc == -ENODEV) {
err ("You chose to insert Single Bridge, or nested bridges, this is not supported...\n");
err ("Bus %x, devfunc %x\n", cur_func->busno, cur_func->device);
return rc;
}
if (rc) {
/* We need to do this in case some other BARs were properly inserted */
func->bus = 1; /* To indicate to the unconfigure function that this is a PPB */
err ("was not able to hot-add PPB properly.\n");
cleanup_count = 2;
goto error;
}
debug ("cur_func->busno = %x, device = %x, function = %x\n",
cur_func->busno, device, function);
pci_bus_read_config_byte (ibmphp_pci_bus, devfn, PCI_SECONDARY_BUS, &sec_number);
debug ("after configuring bridge..., sec_number = %x\n", sec_number);
for (i = 0; i < 32; i++) {
if (!func->devices[i])
continue;
debug ("inside for loop, device is %x\n", i);
newfunc = append(cur_func, sec_number, i);
if (!newfunc)
return -ENOMEM;
rc = ibmphp_configure_card (newfunc, slotno);
/* Again, this case should not happen... For complete paranoia, will need to call remove_bus */
if (rc) {
/* We need to do this in case some other BARs were properly inserted */
func->bus = 1; /* To indicate to the unconfigure function that this is a PPB */
cleanup_count = 2;
goto error;
}
}
function = 0x8;
break;
default:
err ("MAJOR PROBLEM!!!!, header type not supported? %x\n", hdr_type);
return -ENXIO;
break;
} /* end of switch */
} /* end of for */
if (!valid_device) {
err ("Cannot find any valid devices on the card. Or unable to read from card.\n");
return -ENODEV;
}
return 0;
error:
for (i = 0; i < cleanup_count; i++) {
if (cur_func->io[i]) {
ibmphp_remove_resource (cur_func->io[i]);
cur_func->io[i] = NULL;
} else if (cur_func->pfmem[i]) {
ibmphp_remove_resource (cur_func->pfmem[i]);
cur_func->pfmem[i] = NULL;
} else if (cur_func->mem[i]) {
ibmphp_remove_resource (cur_func->mem[i]);
cur_func->mem[i] = NULL;
}
}
return rc;
}
Am 26.09.2013 04:11, schrieb Al Viro:
> On Wed, Sep 25, 2013 at 12:10:57AM +0200, Alexander Holler wrote:
>
>> Sure and I'm the last one who wants that people do have to use
>> anything else than i for simple loop counters. And allowing longer
>> lines doesn't mean people have to use long names, it allows them use
>> them (if it makes sense). That's a big difference.
>>
>> On the other side it's almost impossible to use verbose variable or
>> function names where they would make sense. Not to speak about all
>> the ugly splitted lines just to be below that ancient CGA limit.
>
> Yeah, the things people will do to avoid <gasp> not nesting the living
> hell out of their code...
>
> Tell you what - pick a random place where the code is nested 8 levels
> deep and I'm fairly sure that you will end up with your finger on one
> hell of ugliness. Ugliness in logical structure, not in forced line
> breaks. Let's experiment... Aha. In drivers/pci/hotplug/ibmphp_pci.c,
> 258 lines of deeply indented shite^Wcode that must be good since it compiles:
>
> <avert your eyes if you've got a weak stomach>
I'm aware of people which do nest 8 levels deep just to avoid a return,
break or goto.
But trying to limit that by limiting the line length is like ...
(choose your own own misguided comparison, it's too late for me I
currently only meorize some of those which don't make sense in english)
Regards,
Alexander Holler
Am 26.09.2013 04:52, schrieb Alexander Holler:
> I'm aware of people which do nest 8 levels deep just to avoid a return,
> break or goto.
>
> But trying to limit that by limiting the line length is like ...
> (choose your own own misguided comparison, it's too late for me I
> currently only meorize some of those which don't make sense in english)
But I'm still able to offer a solution: ;)
limit the number of tabs, not the line length (at least not to 80).
Regards,
Alexander Holler
On Thu, Sep 26, 2013 at 04:57:32AM +0200, Alexander Holler wrote:
> Am 26.09.2013 04:52, schrieb Alexander Holler:
>
> > I'm aware of people which do nest 8 levels deep just to avoid a return,
> > break or goto.
> >
> > But trying to limit that by limiting the line length is like ...
> > (choose your own own misguided comparison, it's too late for me I
> > currently only meorize some of those which don't make sense in english)
>
> But I'm still able to offer a solution: ;)
>
> limit the number of tabs, not the line length (at least not to 80).
With that limited (and it's visually harder to keep track of), what's
the problem with 80-column limit on line length? Just how long do
you want those "descriptive names" to be?
Am 26.09.2013 05:04, schrieb Al Viro:
> On Thu, Sep 26, 2013 at 04:57:32AM +0200, Alexander Holler wrote:
>> Am 26.09.2013 04:52, schrieb Alexander Holler:
>>
>>> I'm aware of people which do nest 8 levels deep just to avoid a return,
>>> break or goto.
>>>
>>> But trying to limit that by limiting the line length is like ...
>>> (choose your own own misguided comparison, it's too late for me I
>>> currently only meorize some of those which don't make sense in english)
>>
>> But I'm still able to offer a solution: ;)
>>
>> limit the number of tabs, not the line length (at least not to 80).
>
> With that limited (and it's visually harder to keep track of), what's
> the problem with 80-column limit on line length? Just how long do
> you want those "descriptive names" to be?
Oh, personally I don't have any limit there. ;) I like descriptive
function and variable names whenever they make sense. And often they
make comments uneccessary and therefor prevent errors because those
descriptive names are visible whenever the function or variable is used,
and comments usually appear only once and get forgotten when scrolled
out of the screen.
But just take a function like
void get_xtime_and_monotonic_and_sleep_offset(struct timespec *xtim,
struct timespec *wtom, struct timespec
*sleep);
I like such function names ;) (ok I wouldn't have use those and), but
it's hard to press this into 80 characters, especially when the
arguments should have some meaning too (e.g. what does wtom stand for?)
If you use that somewhere you get
get_xtime_and_monotonic_and_sleep_offset(a, b, c)
using silly names and that already is a 58 characters long. So only 22
are left to distribute over 3 variable names. And now think what happens
if that wouldn't be a void function.
Regards,
Alexander Holler
On Thu, Sep 26, 2013 at 05:27:15AM +0200, Alexander Holler wrote:
> Oh, personally I don't have any limit there. ;) I like descriptive
> function and variable names whenever they make sense. And often they
> make comments uneccessary and therefor prevent errors because those
> descriptive names are visible whenever the function or variable is
> used, and comments usually appear only once and get forgotten when
> scrolled out of the screen.
>
> But just take a function like
>
> void get_xtime_and_monotonic_and_sleep_offset(struct timespec *xtim,
> struct timespec *wtom, struct
> timespec *sleep);
Charming... Now, try to tell one such name from another, when the
only difference is buried in the middle of long phrase. And yes,
I've seen mistakes clearly of that origin. Made them myself, actually.
Am 26.09.2013 05:48, schrieb Al Viro:
> On Thu, Sep 26, 2013 at 05:27:15AM +0200, Alexander Holler wrote:
>
>> Oh, personally I don't have any limit there. ;) I like descriptive
>> function and variable names whenever they make sense. And often they
>> make comments uneccessary and therefor prevent errors because those
>> descriptive names are visible whenever the function or variable is
>> used, and comments usually appear only once and get forgotten when
>> scrolled out of the screen.
>>
>> But just take a function like
>>
>> void get_xtime_and_monotonic_and_sleep_offset(struct timespec *xtim,
>> struct timespec *wtom, struct
>> timespec *sleep);
>
> Charming... Now, try to tell one such name from another, when the
> only difference is buried in the middle of long phrase. And yes,
> I've seen mistakes clearly of that origin. Made them myself, actually.
>
Nothing is perfect. But the source of the discussion was that I don't
aggree that limiting the line length makes code more simple.
E.g. In your previous example they could have used some verbose name for
"flag" without having to cross an obvious non-existing limit. Such the
author might have seen the "problem" early himself. And I think we all
do sometimes write silly code, even when we should know it better.
E.g. my first version of something like your example don't have
necessarily been better as I'm usually first write something down which
I believe should work, not taking care about anything but functionality.
Then I take a break and have a second look in such a way like you just
have exercised it. And I think most people are unable to write perfect
code right out of their fingers. Of course, I think I got much better in
avoiding deep nesting right from the beginning, but I'm sure I still
write sometimes stupid code. And then there are those time constraints
one just has to withstand, besides the fact that it happens sometimes
that I just don't won't to have a look at my own code again. (The last
limit is often reached by endless reviews with comments to remove a
space here and rename a variable there). Nothing is more annoying than
rewriting source until it looks like if the commenter has written it.
People do think differently, people see code differently and people
write code differently and trying to unify that with unnecessary rules
just annoys almost everyone. I do like it if I can tell who has written
some code by just looking at it, at least if it is readable and isn't in
some obvious uglyand hard to read and hard to understand state.
n8,
Alexander Holler
On Thu, 26 Sep 2013, Alexander Holler wrote:
> Am 26.09.2013 05:04, schrieb Al Viro:
> > On Thu, Sep 26, 2013 at 04:57:32AM +0200, Alexander Holler wrote:
> > > Am 26.09.2013 04:52, schrieb Alexander Holler:
> > >
> > > > I'm aware of people which do nest 8 levels deep just to avoid a return,
> > > > break or goto.
> > > >
> > > > But trying to limit that by limiting the line length is like ...
> > > > (choose your own own misguided comparison, it's too late for me I
> > > > currently only meorize some of those which don't make sense in english)
> > >
> > > But I'm still able to offer a solution: ;)
> > >
> > > limit the number of tabs, not the line length (at least not to 80).
> >
> > With that limited (and it's visually harder to keep track of), what's
> > the problem with 80-column limit on line length? Just how long do
> > you want those "descriptive names" to be?
>
> Oh, personally I don't have any limit there. ;) I like descriptive function
> and variable names whenever they make sense. And often they make comments
> uneccessary and therefor prevent errors because those descriptive names are
> visible whenever the function or variable is used, and comments usually appear
> only once and get forgotten when scrolled out of the screen.
>
> But just take a function like
>
> void get_xtime_and_monotonic_and_sleep_offset(struct timespec *xtim,
> struct timespec *wtom, struct timespec
> *sleep);
>
> I like such function names ;) (ok I wouldn't have use those and), but it's
> hard to press this into 80 characters, especially when the arguments should
> have some meaning too (e.g. what does wtom stand for?)
>
> If you use that somewhere you get
>
> get_xtime_and_monotonic_and_sleep_offset(a, b, c)
>
> using silly names and that already is a 58 characters long. So only 22 are
> left to distribute over 3 variable names. And now think what happens if that
> wouldn't be a void function.
Personally, I prefer to use my screen real estate for multiple 80-column
windows, so I can see different parts of the code at once. Anything that
goes over 80 columns is very hard to read.
Perhaps it is a bad example, but I don't even find this very long name
very understandable. Monotonic is an adjective and xtime and sleep are
nouns, so I don't understand how it all fits together. Maybe cramming a
lot of information into a variable name is not always so successful...
Actually, I really appreciate comments on functions, that explain the
purpose of the function, and the constraints on its usage.
julia
Am 26.09.2013 07:53, schrieb Julia Lawall:
>> void get_xtime_and_monotonic_and_sleep_offset(struct timespec *xtim,
>> struct timespec *wtom, struct timespec
>> *sleep);
>>
>> I like such function names ;) (ok I wouldn't have use those and), but it's
>> hard to press this into 80 characters, especially when the arguments should
>> have some meaning too (e.g. what does wtom stand for?)
>>
>> If you use that somewhere you get
>>
>> get_xtime_and_monotonic_and_sleep_offset(a, b, c)
>>
>> using silly names and that already is a 58 characters long. So only 22 are
>> left to distribute over 3 variable names. And now think what happens if that
>> wouldn't be a void function.
>
> Personally, I prefer to use my screen real estate for multiple 80-column
> windows, so I can see different parts of the code at once. Anything that
> goes over 80 columns is very hard to read.
>
> Perhaps it is a bad example, but I don't even find this very long name
> very understandable. Monotonic is an adjective and xtime and sleep are
> nouns, so I don't understand how it all fits together. Maybe cramming a
It just was the first long function name I came about. And sometimes a
bit background information helps a lot. So without knowing anything else
about that function, I assume the monotonic time is meant and the author
didn't want to add _time_ to the name because it already is long. In
case of the wtom I would think it's from a second author and I still
have no clue what it might be. Maybe I'm missing some backgroud
information here too and should actually read the description of the
function, if there is any. ;)
> lot of information into a variable name is not always so successful...
> Actually, I really appreciate comments on functions, that explain the
> purpose of the function, and the constraints on its usage.
I didn't want do suggest getting rid of (necessary or helpful) comments.
Regards,
Alexander Holler