netdev - RE: [PATCH 2/2] net: qmi_wwan: fix checkpatch warnings

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: <AE90C24D6B3A694183C094C60CF0A2F6026B735F@saturn3.aculab.com>
Date:	Thu, 26 Sep 2013 12:29:06 +0100
From:	"David Laight" <David.Laight@...LAB.COM>
To:	Bjørn Mork <bjorn@...k.no>,
	"Fabio Porcedda" <fabio.porcedda@...il.com>
Cc:	<netdev@...r.kernel.org>, <linux-usb@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	"Dan Williams" <dcbw@...hat.com>
Subject: RE: [PATCH 2/2] net: qmi_wwan: fix checkpatch warnings

> Anything that breaks a previously unbroken argument list will reduce the
> readability in my opinion.  The lines can of course not be unlimited,
> but there is no need to set the limit as low as 80 columns.  Feedback
> I've got from developers using e.g. 80 column braille devices is that
> longer lines isn't really a problem for them either.

The main reason for limiting the line length is so that things look
'sensible' when you have a lot of screen windows displaying different
files. You don't want wrapped code, and you definitely don't want
the RHS of long lines hidden.
With a 1600x1200 monitor I'll display six 80x40 windows (and probably
have some more partially visible ones).

Personally I indent continuation lines by 4 chars if using 8 char
'normal' indentation and 8 chars if using 4. This gives a lot more
room on the continuation lines than the Linux 'line up with the
previous line'.

> This is the only one of your code changes which I can be convinced to
> agreeing may improve readability:
> 
> -     if ((on && || (!on && atomic_dec_and_test(&info->pmcount))) {
> +     if ((on && atomic_add_return(1, &info->pmcount) == 1) ||
> +         (!on && atomic_dec_and_test(&info->pmcount))) {

That can be written succinctly as:
	if (on ? atomic_add_return(1, &info->pmcount) == 1)
	       : atomic_dec_and_test(&info->pmcount)) {
although that construct is somewhat frowned upon!

	David