Kernel Newbies archive on lore.kernel.org
 help / color / Atom feed
From: "Valdis Klētnieks" <valdis.kletnieks@vt.edu>
To: Tomek The Messenger <tomekthemessenger@gmail.com>
Cc: kernelnewbies@kernelnewbies.org
Subject: Re: linux kernel coding style and checkpatch.pl script
Date: Thu, 26 Mar 2020 00:13:04 -0400
Message-ID: <98202.1585195984@turing-police> (raw)
In-Reply-To: <CAA4NGys4OphSLnyoRE9MN-_yQ5kFykr-NGdRW+RxR02TyYOr+g@mail.gmail.com>

[-- Warning: decoded text below may be mangled --]
[-- Attachment #1.1: Type: text/plain; charset=utf-8, Size: 3457 bytes --]

On Wed, 25 Mar 2020 10:36:08 +0100, Tomek The Messenger said:

> There is checkpatch.pl script

To borrow from Pirates of the Carribean, "They're not exactly rules, they're
more like... suggestions..."

Checkpatch flags *possible* code style problems, but it's not perfect. There's
often good reason to ignore them.   For example:

> However can I ignore warning message?
> WARNING: quoted string split across lines
> #974: FILE: fpgax67-core.c:974:
> +               dev_err(&pdev->dev, "registration not done, driver is already "
> +                                                       "registered\n");
>
> If I don't split line I will have another warning that 80 characters is
> exceeded.

Blech. Ick. <vomiting sounds>

Don't split literal strings, it means that grepping the source tree for
"already registered" fails. Making grep for a string work is more important
than shutting up checkpatch.

> For sure I can ignore warnings about:
> WARNING: struct  should normally be const
> #998: FILE: fpgax67-core.c :998:

This one you actually need to look at what the routine says.  The object is
*usually* a const - but might not be.  Figure out what it actually does.

Always keep in mind that it's a perl script, and just doing regex matching. It
has no clue what the code actually does. Hopefully you have more understanding
of the code than the perl script does...

> For sure all errors must be fixed like:
> const char* tmp -> change to -> const char *tmp;
> if(  => if (   #insert space

If this is new code you're writing, you should fix it before you submit the
patch adding the code. (Unless of course, checkpatch is wrong about a
variable needing to be a const, or similar)

If you're doing major changes to existing code anyhow, a cleanup patch first is
often appropriate.

If you're just fixing checkpatch warnings for the sake of fixing checkpatch
warnings, keep in mind that many maintainers won't accept patches that just
clean up checkpatch, for several reasons:

First, if it's code that's been static for a while (years, sometimes), there's
always a danger that a patch breaks something.  No reason to touch stable code.

If it's code that somebody else is working on, your patch can cause merge
errors with the other person's (which is why only the person doing the other
work should do cleanup patches - they won't conflict with their own work).

Also, it messes up the git history -  consider a patch that changes
an 'if( foo && !bar) {'  to 'if( foo && !baz){'  to fix a bug where the wrong
variable was being tested.  You then submit a patch to fix the space.
Now a 'git blame' on the file shows your patch rather than the one that
fixes the bug.

However, I have it on good authority that Greg KH will cheerfully accept
checkpatch fixes for anything under 'drivers/staging', because that code
is usually in such bad shape that fixups are needed. :)

Personally, I do kernel builds with sparse and extra gcc warnings, and submit
patches only after applying some thought and concluding things like "Yes,
sparse and/or gcc were correct, the variable *should* be static so it can't be
accessed from another module due to a namespace collision".

In other words, the patch should *never* be "Fix a checkpatch/sparse/gcc
complaint". It should always be "Make an objective improvement to the code
that happened to be pointed out by static analysis tools".


[-- Attachment #1.2: Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

  parent reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-25  9:36 Tomek The Messenger
2020-03-25  9:51 ` Greg KH
2020-03-26  4:13 ` Valdis Klētnieks [this message]
2020-03-26 11:36   ` Konstantin Andreev
     [not found]   ` <E1jHQrV-0002Zv-Ln@shelob.surriel.com>
2020-03-26 12:06     ` Greg KH
2020-03-26 13:01       ` Konstantin Andreev
     [not found]       ` <E1jHS2D-0004t2-IX@shelob.surriel.com>
2020-03-26 13:17         ` Greg KH
2020-03-26 10:57 ` Anuz Pratap Singh Tomar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=98202.1585195984@turing-police \
    --to=valdis.kletnieks@vt.edu \
    --cc=kernelnewbies@kernelnewbies.org \
    --cc=tomekthemessenger@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Kernel Newbies archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kernelnewbies/0 kernelnewbies/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kernelnewbies kernelnewbies/ https://lore.kernel.org/kernelnewbies \
		kernelnewbies@kernelnewbies.org
	public-inbox-index kernelnewbies

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernelnewbies.kernelnewbies


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git