linux-kernel - Re: checkpatch guide for newbies

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CA+MoWDpwx_2kh9bEARh4D5xzyYhWekAceipf0YN=3a+Q5W4eNQ@mail.gmail.com>
Date:	Mon, 23 Sep 2013 14:46:38 +0200
From:	Peter Senna Tschudin <peter.senna@...il.com>
To:	Dan Carpenter <dan.carpenter@...cle.com>
Cc:	kernel-janitors@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: checkpatch guide for newbies

On Mon, Sep 23, 2013 at 11:01 AM, Dan Carpenter
<dan.carpenter@...cle.com> 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 majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/